2024-03-26 01:46:13

by Inochi Amaoto

[permalink] [raw]
Subject: [PATCH v5 0/3] riscv: sophgo: add dmamux support for Sophgo CV1800/SG2000 SoCs

Add dma multiplexer support for the Sophgo CV1800/SG2000 SoCs.

Changed from v4:
1. remove the syscon binding since it can not be complete (still
lack some subdevices)
2. add reg description for the binding,
3. remove the fixed channel assign for dmamux binding
3. driver adopt to the binding change. Now the driver allocates all the
channel when initing and maps the request chan to the channel dynamicly.

Changed from v3:
1. fix dt-binding address issue.

Changed from v2:
1. add reg property of dmamux node in the binding of patch 2

Changed from v1:
1. fix wrong title of patch 2.

Inochi Amaoto (3):
dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC
soc/sophgo: add top sysctrl layout file for CV18XX/SG200X
dmaengine: add driver for Sophgo CV18XX/SG200X dmamux

.../bindings/dma/sophgo,cv1800-dmamux.yaml | 48 ++++
drivers/dma/Kconfig | 9 +
drivers/dma/Makefile | 1 +
drivers/dma/cv1800-dmamux.c | 268 ++++++++++++++++++
include/dt-bindings/dma/cv1800-dma.h | 55 ++++
include/soc/sophgo/cv1800-sysctl.h | 30 ++
6 files changed, 411 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml
create mode 100644 drivers/dma/cv1800-dmamux.c
create mode 100644 include/dt-bindings/dma/cv1800-dma.h
create mode 100644 include/soc/sophgo/cv1800-sysctl.h

--
2.44.0



2024-03-26 01:47:19

by Inochi Amaoto

[permalink] [raw]
Subject: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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 <[email protected]>
---
.../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

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 <[email protected]>
+
+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__
--
2.44.0


2024-03-26 02:11:27

by Inochi Amaoto

[permalink] [raw]
Subject: [PATCH v5 2/3] soc/sophgo: add top sysctrl layout file for CV18XX/SG200X

The "top" system controller of CV18XX/SG200X exposes control
register access for various devices. Add soc header file to
describe it.

Signed-off-by: Inochi Amaoto <[email protected]>
---
include/soc/sophgo/cv1800-sysctl.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 include/soc/sophgo/cv1800-sysctl.h

diff --git a/include/soc/sophgo/cv1800-sysctl.h b/include/soc/sophgo/cv1800-sysctl.h
new file mode 100644
index 000000000000..b9396d33e240
--- /dev/null
+++ b/include/soc/sophgo/cv1800-sysctl.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2023 Inochi Amaoto <[email protected]>
+ */
+
+#ifndef CV1800_SYSCTL_H
+#define CV1800_SYSCTL_H
+
+/*
+ * SOPHGO CV1800/SG2000 SoC top system controller registers offsets.
+ */
+
+#define CV1800_CONF_INFO 0x004
+#define CV1800_SYS_CTRL_REG 0x008
+#define CV1800_USB_PHY_CTRL_REG 0x048
+#define CV1800_SDMA_DMA_CHANNEL_REMAP0 0x154
+#define CV1800_SDMA_DMA_CHANNEL_REMAP1 0x158
+#define CV1800_TOP_TIMER_CLK_SEL 0x1a0
+#define CV1800_TOP_WDT_CTRL 0x1a8
+#define CV1800_DDR_AXI_URGENT_OW 0x1b8
+#define CV1800_DDR_AXI_URGENT 0x1bc
+#define CV1800_DDR_AXI_QOS_0 0x1d8
+#define CV1800_DDR_AXI_QOS_1 0x1dc
+#define CV1800_SD_PWRSW_CTRL 0x1f4
+#define CV1800_SD_PWRSW_TIME 0x1f8
+#define CV1800_DDR_AXI_QOS_OW 0x23c
+#define CV1800_SD_CTRL_OPT 0x294
+#define CV1800_SDMA_DMA_INT_MUX 0x298
+
+#endif // CV1800_SYSCTL_H
--
2.44.0


2024-03-26 02:11:36

by Inochi Amaoto

[permalink] [raw]
Subject: [PATCH v5 3/3] dmaengine: add driver for Sophgo CV18XX/SG200X dmamux

Sophgo CV18XX/SG200X use DW AXI CORE with a multiplexer for remapping
its request lines. The multiplexer supports at most 8 request lines.

Add driver for Sophgo CV18XX/SG200X DMA multiplexer.

Signed-off-by: Inochi Amaoto <[email protected]>
---
drivers/dma/Kconfig | 9 ++
drivers/dma/Makefile | 1 +
drivers/dma/cv1800-dmamux.c | 268 ++++++++++++++++++++++++++++++++++++
3 files changed, 278 insertions(+)
create mode 100644 drivers/dma/cv1800-dmamux.c

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 002a5ec80620..cb31520b9f86 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -546,6 +546,15 @@ config PLX_DMA
These are exposed via extra functions on the switch's
upstream port. Each function exposes one DMA channel.

+config SOPHGO_CV1800_DMAMUX
+ tristate "Sophgo CV1800/SG2000 series SoC DMA multiplexer support"
+ depends on MFD_SYSCON
+ depends on ARCH_SOPHGO
+ help
+ Support for the DMA multiplexer on Sophgo CV1800/SG2000
+ series SoCs.
+ Say Y here if your board have this soc.
+
config STE_DMA40
bool "ST-Ericsson DMA40 support"
depends on ARCH_U8500
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index dfd40d14e408..7465f249ee47 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PPC_BESTCOMM) += bestcomm/
obj-$(CONFIG_PXA_DMA) += pxa_dma.o
obj-$(CONFIG_RENESAS_DMA) += sh/
obj-$(CONFIG_SF_PDMA) += sf-pdma/
+obj-$(CONFIG_SOPHGO_CV1800_DMAMUX) += cv1800-dmamux.o
obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
obj-$(CONFIG_STM32_DMA) += stm32-dma.o
obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o
diff --git a/drivers/dma/cv1800-dmamux.c b/drivers/dma/cv1800-dmamux.c
new file mode 100644
index 000000000000..9c01f157ab97
--- /dev/null
+++ b/drivers/dma/cv1800-dmamux.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Inochi Amaoto <[email protected]>
+ */
+
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/of_dma.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/llist.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+
+#include <soc/sophgo/cv1800-sysctl.h>
+#include <dt-bindings/dma/cv1800-dma.h>
+
+#define DMAMUX_NCELLS 2
+#define MAX_DMA_MAPPING_ID DMA_SPI_NOR1
+#define MAX_DMA_CPU_ID DMA_CPU_C906_1
+#define MAX_DMA_CH_ID 7
+
+#define DMAMUX_INTMUX_REGISTER_LEN 4
+#define DMAMUX_NR_CH_PER_REGISTER 4
+#define DMAMUX_BIT_PER_CH 8
+#define DMAMUX_CH_MASk GENMASK(5, 0)
+#define DMAMUX_INT_BIT_PER_CPU 10
+#define DMAMUX_CH_UPDATE_BIT BIT(31)
+
+#define DMAMUX_CH_REGPOS(chid) \
+ ((chid) / DMAMUX_NR_CH_PER_REGISTER)
+#define DMAMUX_CH_REGOFF(chid) \
+ ((chid) % DMAMUX_NR_CH_PER_REGISTER)
+#define DMAMUX_CH_REG(chid) \
+ ((DMAMUX_CH_REGPOS(chid) * sizeof(u32)) + \
+ CV1800_SDMA_DMA_CHANNEL_REMAP0)
+#define DMAMUX_CH_SET(chid, val) \
+ (((val) << (DMAMUX_CH_REGOFF(chid) * DMAMUX_BIT_PER_CH)) | \
+ DMAMUX_CH_UPDATE_BIT)
+#define DMAMUX_CH_MASK(chid) \
+ DMAMUX_CH_SET(chid, DMAMUX_CH_MASk)
+
+#define DMAMUX_INT_BIT(chid, cpuid) \
+ BIT((cpuid) * DMAMUX_INT_BIT_PER_CPU + (chid))
+#define DMAMUX_INTEN_BIT(cpuid) \
+ DMAMUX_INT_BIT(8, cpuid)
+#define DMAMUX_INT_CH_BIT(chid, cpuid) \
+ (DMAMUX_INT_BIT(chid, cpuid) | DMAMUX_INTEN_BIT(cpuid))
+#define DMAMUX_INT_MASK(chid) \
+ (DMAMUX_INT_BIT(chid, DMA_CPU_A53) | \
+ DMAMUX_INT_BIT(chid, DMA_CPU_C906_0) | \
+ DMAMUX_INT_BIT(chid, DMA_CPU_C906_1))
+#define DMAMUX_INT_CH_MASK(chid, cpuid) \
+ (DMAMUX_INT_MASK(chid) | DMAMUX_INTEN_BIT(cpuid))
+
+struct cv1800_dmamux_data {
+ struct dma_router dmarouter;
+ struct regmap *regmap;
+ spinlock_t lock;
+ struct llist_head free_maps;
+ struct llist_head reserve_maps;
+ DECLARE_BITMAP(mapped_peripherals, MAX_DMA_MAPPING_ID);
+};
+
+struct cv1800_dmamux_map {
+ struct llist_node node;
+ unsigned int channel;
+ unsigned int peripheral;
+ unsigned int cpu;
+};
+
+static void cv1800_dmamux_free(struct device *dev, void *route_data)
+{
+ struct cv1800_dmamux_data *dmamux = dev_get_drvdata(dev);
+ struct cv1800_dmamux_map *map = route_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dmamux->lock, flags);
+
+ regmap_update_bits(dmamux->regmap,
+ DMAMUX_CH_REG(map->channel),
+ DMAMUX_CH_MASK(map->channel),
+ DMAMUX_CH_UPDATE_BIT);
+
+ regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX,
+ DMAMUX_INT_CH_MASK(map->channel, map->cpu),
+ DMAMUX_INTEN_BIT(map->cpu));
+
+ spin_unlock_irqrestore(&dmamux->lock, flags);
+
+ dev_info(dev, "free channel %u for req %u (cpu %u)\n",
+ map->channel, map->peripheral, map->cpu);
+}
+
+static void *cv1800_dmamux_route_allocate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
+ struct cv1800_dmamux_data *dmamux = platform_get_drvdata(pdev);
+ struct cv1800_dmamux_map *map;
+ struct llist_node *node;
+ unsigned long flags;
+ unsigned int chid, devid, cpuid;
+ int ret;
+
+ if (dma_spec->args_count != DMAMUX_NCELLS) {
+ dev_err(&pdev->dev, "invalid number of dma mux args\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ devid = dma_spec->args[0];
+ cpuid = dma_spec->args[1];
+ dma_spec->args_count = 1;
+
+ if (devid > MAX_DMA_MAPPING_ID) {
+ dev_err(&pdev->dev, "invalid device id: %u\n", devid);
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (cpuid > MAX_DMA_CPU_ID) {
+ dev_err(&pdev->dev, "invalid cpu id: %u\n", cpuid);
+ return ERR_PTR(-EINVAL);
+ }
+
+ dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", 0);
+ if (!dma_spec->np) {
+ dev_err(&pdev->dev, "can't get dma master\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ spin_lock_irqsave(&dmamux->lock, flags);
+
+ if (test_bit(devid, dmamux->mapped_peripherals)) {
+ llist_for_each_entry(map, dmamux->reserve_maps.first, node) {
+ if (map->peripheral == devid && map->cpu == cpuid)
+ goto found;
+ }
+
+ ret = -EINVAL;
+ goto failed;
+ } else {
+ node = llist_del_first(&dmamux->free_maps);
+ if (!node) {
+ ret = -ENODEV;
+ goto failed;
+ }
+
+ map = llist_entry(node, struct cv1800_dmamux_map, node);
+ llist_add(&map->node, &dmamux->reserve_maps);
+ set_bit(devid, dmamux->mapped_peripherals);
+ }
+
+found:
+ chid = map->channel;
+ map->peripheral = devid;
+ map->cpu = cpuid;
+
+ 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));
+
+ spin_unlock_irqrestore(&dmamux->lock, flags);
+
+ dma_spec->args[0] = chid;
+
+ dev_info(&pdev->dev, "register channel %u for req %u (cpu %u)\n",
+ chid, devid, cpuid);
+
+ return map;
+
+failed:
+ spin_unlock_irqrestore(&dmamux->lock, flags);
+ of_node_put(dma_spec->np);
+ dev_err(&pdev->dev, "errno %d\n", ret);
+ return ERR_PTR(ret);
+
+}
+
+static int cv1800_dmamux_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *mux_node = dev->of_node;
+ struct cv1800_dmamux_data *data;
+ struct cv1800_dmamux_map *tmp;
+ struct device *parent = dev->parent;
+ struct device_node *dma_master;
+ struct regmap *regmap = NULL;
+ unsigned int i;
+
+ if (!parent)
+ return -ENODEV;
+
+ regmap = device_node_to_regmap(parent->of_node);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ dma_master = of_parse_phandle(mux_node, "dma-masters", 0);
+ if (!dma_master) {
+ dev_err(dev, "invalid dma-requests property\n");
+ return -ENODEV;
+ }
+ of_node_put(dma_master);
+
+ data = devm_kmalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ spin_lock_init(&data->lock);
+ init_llist_head(&data->free_maps);
+
+ for (i = 0; i <= MAX_DMA_CH_ID; i++) {
+ tmp = devm_kmalloc(dev, sizeof(*tmp), GFP_KERNEL);
+ if (!tmp) {
+ /* It is OK for not allocating all channel */
+ dev_warn(dev, "can not allocate channel %u\n", i);
+ continue;
+ }
+
+ init_llist_node(&tmp->node);
+ tmp->channel = i;
+ llist_add(&tmp->node, &data->free_maps);
+ }
+
+ /* if no channel is allocated, the probe must fail */
+ if (llist_empty(&data->free_maps))
+ return -ENOMEM;
+
+ data->regmap = regmap;
+ data->dmarouter.dev = dev;
+ data->dmarouter.route_free = cv1800_dmamux_free;
+
+ platform_set_drvdata(pdev, data);
+
+ return of_dma_router_register(mux_node,
+ cv1800_dmamux_route_allocate,
+ &data->dmarouter);
+}
+
+static void cv1800_dmamux_remove(struct platform_device *pdev)
+{
+ of_dma_controller_free(pdev->dev.of_node);
+}
+
+static const struct of_device_id cv1800_dmamux_ids[] = {
+ { .compatible = "sophgo,cv1800-dmamux", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, cv1800_dmamux_ids);
+
+static struct platform_driver cv1800_dmamux_driver = {
+ .driver = {
+ .name = "cv1800-dmamux",
+ .of_match_table = cv1800_dmamux_ids,
+ },
+ .probe = cv1800_dmamux_probe,
+ .remove_new = cv1800_dmamux_remove,
+};
+module_platform_driver(cv1800_dmamux_driver);
+
+MODULE_AUTHOR("Inochi Amaoto <[email protected]>");
+MODULE_DESCRIPTION("Sophgo CV1800/SG2000 Series Soc DMAMUX driver");
+MODULE_LICENSE("GPL");
--
2.44.0


2024-03-26 03:38:41

by Frank Li

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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
>

2024-03-26 03:49:23

by Inochi Amaoto

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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 <[email protected]>
> > ---
> > .../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 <[email protected]>
> > +
> > +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
> >

2024-03-26 06:51:05

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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 <[email protected]>
>> ---
>> .../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


2024-03-26 06:51:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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


2024-03-26 06:58:00

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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


2024-03-26 07:36:08

by Inochi Amaoto

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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
>

2024-03-26 08:53:24

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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


2024-03-26 11:15:33

by Inochi Amaoto

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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

2024-03-26 11:31:34

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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


2024-03-26 11:42:08

by Inochi Amaoto

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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.

2024-03-26 11:57:23

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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


2024-03-26 12:06:18

by Inochi Amaoto

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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
>

2024-03-26 12:15:20

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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


2024-03-26 12:19:45

by Inochi Amaoto

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

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