[PATCH 0/2] Add MediaTek DRAMC driver support

[PATCH 0/2] Add MediaTek DRAMC driver support

[Crystal Guo]

This series is based on linux-next, tag: next-20241210.

Vcore DVFS feature need know the current DDR data rate.
Add MediaTek DRAMC driver to provide an interface that can
obtain current DDR data rate.

Crystal Guo (2):
  memory/mediatek: Add an interface to get current DDR data rate
  dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

 .../mediatek,common-dramc.yaml                | 129 +++++++
 drivers/memory/Kconfig                        |   1 +
 drivers/memory/Makefile                       |   1 +
 drivers/memory/mediatek/Kconfig               |  21 ++
 drivers/memory/mediatek/Makefile              |   2 +
 drivers/memory/mediatek/mtk-dramc.c           | 325 ++++++++++++++++++
 include/linux/soc/mediatek/mtk-dramc.h        |  41 +++
 7 files changed, 520 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
 create mode 100644 drivers/memory/mediatek/Kconfig
 create mode 100644 drivers/memory/mediatek/Makefile
 create mode 100644 drivers/memory/mediatek/mtk-dramc.c
 create mode 100644 include/linux/soc/mediatek/mtk-dramc.h

-- 
2.18.0

[PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[Crystal Guo]

Add MediaTek DRAMC driver to provide an interface that can
obtain current DDR data rate.

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 drivers/memory/Kconfig                 |   1 +
 drivers/memory/Makefile                |   1 +
 drivers/memory/mediatek/Kconfig        |  21 ++
 drivers/memory/mediatek/Makefile       |   2 +
 drivers/memory/mediatek/mtk-dramc.c    | 325 +++++++++++++++++++++++++
 include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
 6 files changed, 391 insertions(+)
 create mode 100644 drivers/memory/mediatek/Kconfig
 create mode 100644 drivers/memory/mediatek/Makefile
 create mode 100644 drivers/memory/mediatek/mtk-dramc.c
 create mode 100644 include/linux/soc/mediatek/mtk-dramc.h

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index c82d8d8a16ea..b1698549ff81 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -227,5 +227,6 @@ config STM32_FMC2_EBI
 
 source "drivers/memory/samsung/Kconfig"
 source "drivers/memory/tegra/Kconfig"
+source "drivers/memory/mediatek/Kconfig"
 
 endif
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index d2e6ca9abbe0..cf1091449d2e 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)	+= stm32-fmc2-ebi.o
 
 obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
 obj-$(CONFIG_TEGRA_MC)		+= tegra/
+obj-$(CONFIG_HAVE_MTK_MC)	+= mediatek/
 obj-$(CONFIG_TI_EMIF_SRAM)	+= ti-emif-sram.o
 obj-$(CONFIG_FPGA_DFL_EMIF)	+= dfl-emif.o
 
diff --git a/drivers/memory/mediatek/Kconfig b/drivers/memory/mediatek/Kconfig
new file mode 100644
index 000000000000..00764cdb157e
--- /dev/null
+++ b/drivers/memory/mediatek/Kconfig
@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config HAVE_MTK_MC
+	bool "MediaTek Memory Controller support"
+	help
+	  This option allows to enable MediaTek memory controller drivers,
+	  which may include controllers for DRAM or others.
+	  Select Y here if you need support for MediaTek memory controller.
+	  If you don't need, select N.
+
+if HAVE_MTK_MC
+
+config MTK_DRAMC
+	tristate "MediaTek DRAMC driver"
+	depends on HAVE_MTK_MC
+	help
+	  This option selects the MediaTek DRAMC driver, which provides
+	  an interface for reporting the current data rate of DRAM.
+	  Select Y here if you need support for the MediaTek DRAMC driver.
+	  If you don't need, select N.
+
+endif
diff --git a/drivers/memory/mediatek/Makefile b/drivers/memory/mediatek/Makefile
new file mode 100644
index 000000000000..a1395fc55b41
--- /dev/null
+++ b/drivers/memory/mediatek/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MTK_DRAMC)		+= mtk-dramc.o
diff --git a/drivers/memory/mediatek/mtk-dramc.c b/drivers/memory/mediatek/mtk-dramc.c
new file mode 100644
index 000000000000..9c2c8e187a4a
--- /dev/null
+++ b/drivers/memory/mediatek/mtk-dramc.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/printk.h>
+#include <linux/io.h>
+#include <linux/soc/mediatek/mtk-dramc.h>
+
+static struct platform_device *dramc_pdev;
+static struct platform_driver dramc_drv;
+
+static int fmeter_init(struct platform_device *pdev,
+		       struct fmeter_dev_t *fmeter_dev_ptr, unsigned int fmeter_version)
+{
+	struct device_node *dramc_node = pdev->dev.of_node;
+	int ret;
+
+	ret = of_property_read_u32(dramc_node,
+				   "crystal-freq", &(fmeter_dev_ptr->crystal_freq));
+	ret |= of_property_read_u32(dramc_node,
+				    "shu-of", &(fmeter_dev_ptr->shu_of));
+	ret |= of_property_read_u32_array(dramc_node,
+					  "shu-lv", (unsigned int *)&(fmeter_dev_ptr->shu_lv), 3);
+	ret |= of_property_read_u32_array(dramc_node,
+					  "pll-id", (unsigned int *)&(fmeter_dev_ptr->pll_id), 3);
+	ret |= of_property_read_u32_array(dramc_node,
+					  "sdmpcw", (unsigned int *)(fmeter_dev_ptr->sdmpcw), 6);
+	ret |= of_property_read_u32_array(dramc_node,
+					  "posdiv", (unsigned int *)(fmeter_dev_ptr->posdiv), 6);
+	ret |= of_property_read_u32_array(dramc_node,
+					  "fbksel", (unsigned int *)(fmeter_dev_ptr->fbksel), 6);
+	ret |= of_property_read_u32_array(dramc_node,
+					  "dqsopen", (unsigned int *)(fmeter_dev_ptr->dqsopen), 6);
+	if (fmeter_version == 1) {
+		fmeter_dev_ptr->version = 1;
+		ret |= of_property_read_u32_array(dramc_node,
+			"async-ca", (unsigned int *)(fmeter_dev_ptr->async_ca), 6);
+		ret |= of_property_read_u32_array(dramc_node,
+			"dq-ser-mode", (unsigned int *)(fmeter_dev_ptr->dq_ser_mode), 6);
+	}
+	return ret;
+}
+
+static ssize_t dram_data_rate_show(struct device_driver *driver, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "DRAM data rate = %d\n",
+		mtk_dramc_get_data_rate());
+}
+
+static DRIVER_ATTR_RO(dram_data_rate);
+
+static int dramc_probe(struct platform_device *pdev)
+{
+	struct device_node *dramc_node = pdev->dev.of_node;
+	struct dramc_dev_t *dramc_dev_ptr;
+	unsigned int fmeter_version;
+	struct resource *res;
+	unsigned int i, size;
+	int ret;
+
+	pr_info("%s: module probe.\n", __func__);
+	dramc_pdev = pdev;
+	dramc_dev_ptr = devm_kmalloc(&pdev->dev,
+				     sizeof(struct dramc_dev_t), GFP_KERNEL);
+
+	if (!dramc_dev_ptr)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(dramc_node,
+				   "support-ch-cnt", &dramc_dev_ptr->support_ch_cnt);
+	if (ret) {
+		pr_info("%s: get support_ch_cnt fail\n", __func__);
+		return -EINVAL;
+	}
+
+	dramc_dev_ptr->sleep_base = of_iomap(dramc_node,
+					     dramc_dev_ptr->support_ch_cnt * 4);
+	if (IS_ERR(dramc_dev_ptr->sleep_base)) {
+		pr_info("%s: unable to map sleep base\n", __func__);
+		return -EINVAL;
+	}
+
+	size = sizeof(phys_addr_t) * dramc_dev_ptr->support_ch_cnt;
+	dramc_dev_ptr->dramc_chn_base_ao = devm_kmalloc(&pdev->dev,
+							size, GFP_KERNEL);
+	if (!(dramc_dev_ptr->dramc_chn_base_ao))
+		return -ENOMEM;
+	dramc_dev_ptr->dramc_chn_base_nao = devm_kmalloc(&pdev->dev,
+							 size, GFP_KERNEL);
+	if (!(dramc_dev_ptr->dramc_chn_base_nao))
+		return -ENOMEM;
+	dramc_dev_ptr->ddrphy_chn_base_ao = devm_kmalloc(&pdev->dev,
+							 size, GFP_KERNEL);
+	if (!(dramc_dev_ptr->ddrphy_chn_base_ao))
+		return -ENOMEM;
+	dramc_dev_ptr->ddrphy_chn_base_nao = devm_kmalloc(&pdev->dev,
+							  size, GFP_KERNEL);
+	if (!(dramc_dev_ptr->ddrphy_chn_base_nao))
+		return -ENOMEM;
+
+	for (i = 0; i < dramc_dev_ptr->support_ch_cnt; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		dramc_dev_ptr->dramc_chn_base_ao[i] =
+			devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(dramc_dev_ptr->dramc_chn_base_ao[i])) {
+			pr_info("%s: unable to map ch%d DRAMC AO base\n",
+				__func__, i);
+			return -EINVAL;
+		}
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM,
+					    i + dramc_dev_ptr->support_ch_cnt);
+		dramc_dev_ptr->dramc_chn_base_nao[i] =
+			devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(dramc_dev_ptr->dramc_chn_base_nao[i])) {
+			pr_info("%s: unable to map ch%d DRAMC NAO base\n",
+				__func__, i);
+			return -EINVAL;
+		}
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM,
+					    i + dramc_dev_ptr->support_ch_cnt * 2);
+		dramc_dev_ptr->ddrphy_chn_base_ao[i] =
+			devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_ao[i])) {
+			pr_info("%s: unable to map ch%d DDRPHY AO base\n",
+				__func__, i);
+			return -EINVAL;
+		}
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM,
+					    i + dramc_dev_ptr->support_ch_cnt * 3);
+		dramc_dev_ptr->ddrphy_chn_base_nao[i] =
+			devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_nao[i])) {
+			pr_info("%s: unable to map ch%d DDRPHY NAO base\n",
+				__func__, i);
+			return -EINVAL;
+		}
+	}
+
+	ret = of_property_read_u32(dramc_node, "fmeter-version", &fmeter_version);
+	if (ret) {
+		pr_info("%s: get fmeter_version fail\n", __func__);
+		return -EINVAL;
+	}
+	pr_info("%s: fmeter_version(%d)\n", __func__, fmeter_version);
+
+	if (fmeter_version == 1) {
+		dramc_dev_ptr->fmeter_dev_ptr = devm_kmalloc(&pdev->dev,
+							     sizeof(struct fmeter_dev_t),
+							     GFP_KERNEL);
+		if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
+			pr_info("%s: memory  alloc fail\n", __func__);
+			return -ENOMEM;
+		}
+		ret = fmeter_init(pdev, dramc_dev_ptr->fmeter_dev_ptr, fmeter_version);
+		if (ret) {
+			pr_info("%s: fmeter_init fail\n", __func__);
+			return -EINVAL;
+		}
+	} else {
+		dramc_dev_ptr->fmeter_dev_ptr = NULL;
+	}
+	ret = driver_create_file(pdev->dev.driver, &driver_attr_dram_data_rate);
+	if (ret) {
+		pr_info("%s: fail to create dram_data_rate sysfs\n", __func__);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, dramc_dev_ptr);
+	pr_info("%s: DRAM data rate = %d\n", __func__,
+		mtk_dramc_get_data_rate());
+
+	return ret;
+}
+
+static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
+{
+	struct fmeter_dev_t *fmeter_dev_ptr =
+		(struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
+	unsigned int shu_lv_val;
+	unsigned int pll_id_val;
+	unsigned int sdmpcw_val;
+	unsigned int posdiv_val;
+	unsigned int ckdiv4_val;
+	unsigned int offset;
+	unsigned int vco_freq;
+	unsigned int fbksel;
+	unsigned int dqsopen;
+	unsigned int async_ca;
+	unsigned int dq_ser_mode;
+
+	shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
+		fmeter_dev_ptr->shu_lv.offset) &
+		fmeter_dev_ptr->shu_lv.mask) >>
+		fmeter_dev_ptr->shu_lv.shift;
+
+	pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
+		fmeter_dev_ptr->pll_id.offset) &
+		fmeter_dev_ptr->pll_id.mask) >>
+		fmeter_dev_ptr->pll_id.shift;
+
+	offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
+		fmeter_dev_ptr->shu_of * shu_lv_val;
+	sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
+		fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
+		fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
+
+	offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
+		fmeter_dev_ptr->shu_of * shu_lv_val;
+	posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
+		fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
+		fmeter_dev_ptr->posdiv[pll_id_val].shift;
+
+	offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
+		fmeter_dev_ptr->shu_of * shu_lv_val;
+	fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
+		fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
+		fmeter_dev_ptr->fbksel[pll_id_val].shift;
+
+	offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
+		fmeter_dev_ptr->shu_of * shu_lv_val;
+	dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
+		fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
+		fmeter_dev_ptr->dqsopen[pll_id_val].shift;
+
+	offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
+		fmeter_dev_ptr->shu_of * shu_lv_val;
+	async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
+		fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
+		fmeter_dev_ptr->async_ca[pll_id_val].shift;
+
+	offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
+		fmeter_dev_ptr->shu_of * shu_lv_val;
+	dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
+		fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
+		fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
+	ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3: DIV16
+
+	posdiv_val &= ~(0x4);
+
+	vco_freq = ((fmeter_dev_ptr->crystal_freq) *
+		(sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
+		<< fbksel;
+
+	if ((dqsopen == 1) && (async_ca == 1))
+		vco_freq >>= 1;
+
+	return vco_freq;
+}
+
+/*
+ * mtk_dramc_get_data_rate - calculate DRAM data rate
+ *
+ * Returns DRAM data rate (MB/s)
+ */
+unsigned int mtk_dramc_get_data_rate(void)
+{
+	struct dramc_dev_t *dramc_dev_ptr;
+	struct fmeter_dev_t *fmeter_dev_ptr;
+
+	if (!dramc_pdev)
+		return 0;
+
+	dramc_dev_ptr =
+		(struct dramc_dev_t *)platform_get_drvdata(dramc_pdev);
+
+	fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
+	if (!fmeter_dev_ptr)
+		return 0;
+
+	if (fmeter_dev_ptr->version == 1)
+		return fmeter_v1(dramc_dev_ptr);
+	return 0;
+}
+EXPORT_SYMBOL(mtk_dramc_get_data_rate);
+
+static int dramc_remove(struct platform_device *pdev)
+{
+	dramc_pdev = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id dramc_of_ids[] = {
+	{.compatible = "mediatek,common-dramc",},
+	{}
+};
+
+static struct platform_driver dramc_drv = {
+	.probe = dramc_probe,
+	.remove = dramc_remove,
+	.driver = {
+		.name = "dramc_drv",
+		.owner = THIS_MODULE,
+		.of_match_table = dramc_of_ids,
+	},
+};
+
+static int __init dramc_drv_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&dramc_drv);
+	if (ret) {
+		pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+module_init(dramc_drv_init);
+
+MODULE_AUTHOR("Mediatek Corporation");
+MODULE_DESCRIPTION("MediaTek DRAMC Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/soc/mediatek/mtk-dramc.h b/include/linux/soc/mediatek/mtk-dramc.h
new file mode 100644
index 000000000000..95e7dbfe7d0e
--- /dev/null
+++ b/include/linux/soc/mediatek/mtk-dramc.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 MediaTek Inc.
+ */
+
+#ifndef __MTK_DRAMC_H__
+#define __MTK_DRAMC_H__
+
+struct reg_ctrl_t {
+	unsigned int offset;
+	unsigned int mask;
+	unsigned int shift;
+};
+
+struct fmeter_dev_t {
+	unsigned int version;
+	unsigned int crystal_freq;
+	unsigned int shu_of;
+	struct reg_ctrl_t shu_lv;
+	struct reg_ctrl_t pll_id;
+	struct reg_ctrl_t sdmpcw[2];
+	struct reg_ctrl_t posdiv[2];
+	struct reg_ctrl_t fbksel[2];
+	struct reg_ctrl_t dqsopen[2];
+	struct reg_ctrl_t async_ca[2];
+	struct reg_ctrl_t dq_ser_mode[2];
+};
+
+struct dramc_dev_t {
+	unsigned int support_ch_cnt;
+	void __iomem **dramc_chn_base_ao;
+	void __iomem **dramc_chn_base_nao;
+	void __iomem **ddrphy_chn_base_ao;
+	void __iomem **ddrphy_chn_base_nao;
+	void __iomem *sleep_base;
+	void *fmeter_dev_ptr;
+};
+
+unsigned int mtk_dramc_get_data_rate(void);
+
+#endif /* __MTK_DRAMC_H__ */
-- 
2.18.0

[PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo]

Add devicetree binding for mediatek common-dramc driver.

The DRAM controller of MediaTek SoC provides an interface to
get the current data rate of DRAM.

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 .../mediatek,common-dramc.yaml                | 129 ++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
new file mode 100644
index 000000000000..c9e608c7f183
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
@@ -0,0 +1,129 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+# Copyright (c) 2024 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Common DRAMC (DRAM Controller)
+
+maintainers:
+  - Crystal Guo <crystal.guo@mediatek.com>
+
+description: |
+  The DRAM controller of MediaTek SoC provides an interface to
+  get the current data rate of DRAM.
+
+properties:
+  compatible:
+    const: mediatek,common-dramc
+
+  reg:
+    minItems: 9
+    items:
+      - description: DRAMC_AO_CHA_BASE
+      - description: DRAMC_AO_CHB_BASE
+      - description: DRAMC_AO_CHC_BASE
+      - description: DRAMC_AO_CHD_BASE
+      - description: DRAMC_NAO_CHA_BASE
+      - description: DRAMC_NAO_CHB_BASE
+      - description: DRAMC_NAO_CHC_BASE
+      - description: DRAMC_NAO_CHD_BASE
+      - description: DDRPHY_AO_CHA_BASE
+      - description: DDRPHY_AO_CHB_BASE
+      - description: DDRPHY_AO_CHC_BASE
+      - description: DDRPHY_AO_CHD_BASE
+      - description: DDRPHY_NAO_CHA_BASE
+      - description: DDRPHY_NAO_CHB_BASE
+      - description: DDRPHY_NAO_CHC_BASE
+      - description: DDRPHY_NAO_CHD_BASE
+      - description: SLEEP_BASE
+
+  support-ch-cnt:
+    maxItems: 1
+
+  fmeter-version:
+    maxItems: 1
+    description:
+      Fmeter version for calculating dram data rate
+
+  crystal-freq:
+    maxItems: 1
+    description:
+      Reference clock rate in MHz
+
+  shu-of:
+    maxItems: 1
+
+  pll-id: true
+  shu-lv: true
+  sdmpcw: true
+  posdiv: true
+  fbksel: true
+  dqsopen: true
+  async-ca: true
+  dq-ser-mode: true
+
+required:
+  - compatible
+  - reg
+  - support-ch-cnt
+  - fmeter-version
+  - crystal-freq
+  - pll-id
+  - shu-lv
+  - shu-of
+  - sdmpcw
+  - posdiv
+  - fbksel
+  - dqsopen
+  - async-ca
+  - dq-ser-mode
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        dramc: dramc@10230000 {
+            compatible = "mediatek,common-dramc";
+            reg = <0 0x10230000 0 0x2000>, /* DRAMC_AO_CHA_BASE */
+                <0 0x10240000 0 0x2000>, /* DRAMC_AO_CHB_BASE */
+                <0 0x10250000 0 0x2000>, /* DRAMC_AO_CHC_BASE */
+                <0 0x10260000 0 0x2000>, /* DRAMC_AO_CHD_BASE */
+                <0 0x10234000 0 0x1000>, /* DRAMC_NAO_CHA_BASE */
+                <0 0x10244000 0 0x1000>, /* DRAMC_NAO_CHB_BASE */
+                <0 0x10254000 0 0x1000>, /* DRAMC_NAO_CHC_BASE */
+                <0 0x10264000 0 0x1000>, /* DRAMC_NAO_CHD_BASE */
+                <0 0x10238000 0 0x2000>, /* DDRPHY_AO_CHA_BASE */
+                <0 0x10248000 0 0x2000>, /* DDRPHY_AO_CHB_BASE */
+                <0 0x10258000 0 0x2000>, /* DDRPHY_AO_CHC_BASE */
+                <0 0x10268000 0 0x2000>, /* DDRPHY_AO_CHD_BASE */
+                <0 0x10236000 0 0x2000>, /* DDRPHY_NAO_CHA_BASE */
+                <0 0x10246000 0 0x2000>, /* DDRPHY_NAO_CHB_BASE */
+                <0 0x10256000 0 0x2000>, /* DDRPHY_NAO_CHC_BASE */
+                <0 0x10266000 0 0x2000>, /* DDRPHY_NAO_CHD_BASE */
+                <0 0x10006000 0 0x1000>; /* SLEEP_BASE */
+            support-ch-cnt = <4>;
+            fmeter-version = <1>;
+            crystal-freq = <26>;
+            pll-id = <0x0e98 0x02000000 25>;
+            shu-lv = <0x0e98 0x0000c000 14>;
+            shu-of = <0x700>;
+            sdmpcw = <0x0908 0x0007fff8 3>,
+                <0x0928 0x0007fff8 3>;
+            posdiv = <0x090c 0x00003800 11>,
+                <0x092c 0x00003800 11>;
+            fbksel = <0x0910 0x00000040 6>,
+                <0x0910 0x00000040 6>;
+            dqsopen = <0x0d94 0x04000000 26>,
+                <0x0d94 0x04000000 26>;
+            async-ca = <0x0d08 0x00000001 0>,
+                <0x0d08 0x00000001 0>;
+            dq-ser-mode = <0x0dc4 0x00000018 3>,
+                <0x0dc4 0x00000018 3>;
+        };
+    };
-- 
2.18.0

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Rob Herring (Arm)]

On Thu, 12 Dec 2024 16:59:48 +0800, Crystal Guo wrote:
> Add devicetree binding for mediatek common-dramc driver.
> 
> The DRAM controller of MediaTek SoC provides an interface to
> get the current data rate of DRAM.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>  .../mediatek,common-dramc.yaml                | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: support-ch-cnt: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: fmeter-version: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: crystal-freq: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: shu-of: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: pll-id: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: shu-lv: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: sdmpcw: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: posdiv: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: fbksel: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: dqsopen: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: async-ca: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: dq-ser-mode: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241212090029.13692-3-crystal.guo@mediatek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[AngeloGioacchino Del Regno]

Il 12/12/24 09:59, Crystal Guo ha scritto:
> Add devicetree binding for mediatek common-dramc driver.
> 
> The DRAM controller of MediaTek SoC provides an interface to
> get the current data rate of DRAM.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>   .../mediatek,common-dramc.yaml                | 129 ++++++++++++++++++
>   1 file changed, 129 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> new file mode 100644
> index 000000000000..c9e608c7f183
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +# Copyright (c) 2024 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Common DRAMC (DRAM Controller)

MediaTek DRAM Controller (DRAMC)

> +
> +maintainers:
> +  - Crystal Guo <crystal.guo@mediatek.com>
> +
> +description: |
> +  The DRAM controller of MediaTek SoC provides an interface to
> +  get the current data rate of DRAM.

No, the DRAM Controller does much more than just that.

> +
> +properties:
> +  compatible:
> +    const: mediatek,common-dramc

Absolutely no! Compatibles are per-soc.

mediatek,mt8186-dramc
mediatek,mt8188-dramc
mediatek,mt8195-dramc

etc

> +
> +  reg:
> +    minItems: 9
> +    items:
> +      - description: DRAMC_AO_CHA_BASE
> +      - description: DRAMC_AO_CHB_BASE
> +      - description: DRAMC_AO_CHC_BASE
> +      - description: DRAMC_AO_CHD_BASE

All those channels are sequential in AO->NAO, in the sense that
every channel is:

CH0     AO: 0x10230000   len: 0x4000
CH0    NAO: 0x10234000   len: 0x2000
CH0 PHY_AO: 0x10236000   len: 0x2000
CH0 PHY_AO: 0x10238000   len: 0x2000

So the reg can be simplified as

minItems: 4
items:
   - description: DRAM Controller Channel 0
   - description: DRAM Controller Channel 1
   - description: DRAM Controller Channel 2
   - description: DRAM Controller Channel 3


> +      - description: DRAMC_NAO_CHA_BASE
> +      - description: DRAMC_NAO_CHB_BASE
> +      - description: DRAMC_NAO_CHC_BASE
> +      - description: DRAMC_NAO_CHD_BASE
> +      - description: DDRPHY_AO_CHA_BASE
> +      - description: DDRPHY_AO_CHB_BASE
> +      - description: DDRPHY_AO_CHC_BASE
> +      - description: DDRPHY_AO_CHD_BASE
> +      - description: DDRPHY_NAO_CHA_BASE
> +      - description: DDRPHY_NAO_CHB_BASE
> +      - description: DDRPHY_NAO_CHC_BASE
> +      - description: DDRPHY_NAO_CHD_BASE
> +      - description: SLEEP_BASE

You're not using the SLEEP_BASE iospace, and that's not even really specific
to the DRAM Controller. Drop it.

> +
> +  support-ch-cnt:
> +    maxItems: 1

Don't tell me that the DRAM Controller in MediaTek SoCs cannot see how many
channels are actually occupied by a DRAM bank, because I will be really skeptical.

You can autodetect that in the driver, you don't need a DT property for that.

> +
> +  fmeter-version:
> +    maxItems: 1
> +    description:
> +      Fmeter version for calculating dram data rate

The Fmeter version is SoC-specific, you need platform data, not DT property.

> +
> +  crystal-freq:
> +    maxItems: 1
> +    description:
> +      Reference clock rate in MHz

Is this crystal an external component, or is it integrated into the SoC?

> +
> +  shu-of:
> +    maxItems: 1

There's no description, what is shu-of?

> +
> +  pll-id: true
> +  shu-lv: true
> +  sdmpcw: true
> +  posdiv: true
> +  fbksel: true
> +  dqsopen: true
> +  async-ca: true
> +  dq-ser-mode: true

Same for these ones, please describe them - but then remember: if those parameters
are board-specific, they can stay here, otherwise those go in platform data.

Besides, I doubt that those are board specific.

Regards,
Angelo

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[AngeloGioacchino Del Regno]

Il 12/12/24 09:59, Crystal Guo ha scritto:
> Add MediaTek DRAMC driver to provide an interface that can
> obtain current DDR data rate.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>   drivers/memory/Kconfig                 |   1 +
>   drivers/memory/Makefile                |   1 +
>   drivers/memory/mediatek/Kconfig        |  21 ++
>   drivers/memory/mediatek/Makefile       |   2 +
>   drivers/memory/mediatek/mtk-dramc.c    | 325 +++++++++++++++++++++++++
>   include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
>   6 files changed, 391 insertions(+)
>   create mode 100644 drivers/memory/mediatek/Kconfig
>   create mode 100644 drivers/memory/mediatek/Makefile
>   create mode 100644 drivers/memory/mediatek/mtk-dramc.c
>   create mode 100644 include/linux/soc/mediatek/mtk-dramc.h
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c82d8d8a16ea..b1698549ff81 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -227,5 +227,6 @@ config STM32_FMC2_EBI
>   
>   source "drivers/memory/samsung/Kconfig"
>   source "drivers/memory/tegra/Kconfig"
> +source "drivers/memory/mediatek/Kconfig"
>   
>   endif
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index d2e6ca9abbe0..cf1091449d2e 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)	+= stm32-fmc2-ebi.o
>   
>   obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
>   obj-$(CONFIG_TEGRA_MC)		+= tegra/
> +obj-$(CONFIG_HAVE_MTK_MC)	+= mediatek/
>   obj-$(CONFIG_TI_EMIF_SRAM)	+= ti-emif-sram.o
>   obj-$(CONFIG_FPGA_DFL_EMIF)	+= dfl-emif.o
>   
> diff --git a/drivers/memory/mediatek/Kconfig b/drivers/memory/mediatek/Kconfig
> new file mode 100644
> index 000000000000..00764cdb157e
> --- /dev/null
> +++ b/drivers/memory/mediatek/Kconfig
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config HAVE_MTK_MC
> +	bool "MediaTek Memory Controller support"
> +	help
> +	  This option allows to enable MediaTek memory controller drivers,
> +	  which may include controllers for DRAM or others.
> +	  Select Y here if you need support for MediaTek memory controller.
> +	  If you don't need, select N.
> +
> +if HAVE_MTK_MC
> +
> +config MTK_DRAMC
> +	tristate "MediaTek DRAMC driver"
> +	depends on HAVE_MTK_MC
> +	help
> +	  This option selects the MediaTek DRAMC driver, which provides
> +	  an interface for reporting the current data rate of DRAM.
> +	  Select Y here if you need support for the MediaTek DRAMC driver.
> +	  If you don't need, select N.
> +
> +endif
> diff --git a/drivers/memory/mediatek/Makefile b/drivers/memory/mediatek/Makefile
> new file mode 100644
> index 000000000000..a1395fc55b41
> --- /dev/null
> +++ b/drivers/memory/mediatek/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_MTK_DRAMC)		+= mtk-dramc.o
> diff --git a/drivers/memory/mediatek/mtk-dramc.c b/drivers/memory/mediatek/mtk-dramc.c
> new file mode 100644
> index 000000000000..9c2c8e187a4a
> --- /dev/null
> +++ b/drivers/memory/mediatek/mtk-dramc.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 MediaTek Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/printk.h>
> +#include <linux/io.h>
> +#include <linux/soc/mediatek/mtk-dramc.h>
> +
> +static struct platform_device *dramc_pdev;
> +static struct platform_driver dramc_drv;

You don't need these globals.

> +
> +static int fmeter_init(struct platform_device *pdev,
> +		       struct fmeter_dev_t *fmeter_dev_ptr, unsigned int fmeter_version)
> +{
> +	struct device_node *dramc_node = pdev->dev.of_node;
> +	int ret;
> +
> +	ret = of_property_read_u32(dramc_node,
> +				   "crystal-freq", &(fmeter_dev_ptr->crystal_freq));
> +	ret |= of_property_read_u32(dramc_node,
> +				    "shu-of", &(fmeter_dev_ptr->shu_of));

ret |= x doesn't make sense.

ret = ...
if (ret)
	return ret;

> +	ret |= of_property_read_u32_array(dramc_node,
> +					  "shu-lv", (unsigned int *)&(fmeter_dev_ptr->shu_lv), 3);
> +	ret |= of_property_read_u32_array(dramc_node,
> +					  "pll-id", (unsigned int *)&(fmeter_dev_ptr->pll_id), 3);
> +	ret |= of_property_read_u32_array(dramc_node,
> +					  "sdmpcw", (unsigned int *)(fmeter_dev_ptr->sdmpcw), 6);
> +	ret |= of_property_read_u32_array(dramc_node,
> +					  "posdiv", (unsigned int *)(fmeter_dev_ptr->posdiv), 6);
> +	ret |= of_property_read_u32_array(dramc_node,
> +					  "fbksel", (unsigned int *)(fmeter_dev_ptr->fbksel), 6);
> +	ret |= of_property_read_u32_array(dramc_node,
> +					  "dqsopen", (unsigned int *)(fmeter_dev_ptr->dqsopen), 6);
> +	if (fmeter_version == 1) {
> +		fmeter_dev_ptr->version = 1;
> +		ret |= of_property_read_u32_array(dramc_node,
> +			"async-ca", (unsigned int *)(fmeter_dev_ptr->async_ca), 6);
> +		ret |= of_property_read_u32_array(dramc_node,
> +			"dq-ser-mode", (unsigned int *)(fmeter_dev_ptr->dq_ser_mode), 6);
> +	}
> +	return ret;
> +}
> +
> +static ssize_t dram_data_rate_show(struct device_driver *driver, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "DRAM data rate = %d\n",
> +		mtk_dramc_get_data_rate());
> +}
> +
> +static DRIVER_ATTR_RO(dram_data_rate);> +
> +static int dramc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *dramc_node = pdev->dev.of_node;
> +	struct dramc_dev_t *dramc_dev_ptr;
> +	unsigned int fmeter_version;
> +	struct resource *res;
> +	unsigned int i, size;
> +	int ret;
> +
> +	pr_info("%s: module probe.\n", __func__);

This pr_info makes no sense. Please remove.

> +	dramc_pdev = pdev;
> +	dramc_dev_ptr = devm_kmalloc(&pdev->dev,
> +				     sizeof(struct dramc_dev_t), GFP_KERNEL);
> +
> +	if (!dramc_dev_ptr)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(dramc_node,
> +				   "support-ch-cnt", &dramc_dev_ptr->support_ch_cnt);
> +	if (ret) {
> +		pr_info("%s: get support_ch_cnt fail\n", __func__);

that's an error, and you want dev_err_probe().

> +		return -EINVAL;
> +	}
> +
> +	dramc_dev_ptr->sleep_base = of_iomap(dramc_node,
> +					     dramc_dev_ptr->support_ch_cnt * 4);
> +	if (IS_ERR(dramc_dev_ptr->sleep_base)) {
> +		pr_info("%s: unable to map sleep base\n", __func__);

same, here and everywhere else

> +		return -EINVAL;
> +	}
> +
> +	size = sizeof(phys_addr_t) * dramc_dev_ptr->support_ch_cnt;
> +	dramc_dev_ptr->dramc_chn_base_ao = devm_kmalloc(&pdev->dev,
> +							size, GFP_KERNEL);
> +	if (!(dramc_dev_ptr->dramc_chn_base_ao))
> +		return -ENOMEM;
> +	dramc_dev_ptr->dramc_chn_base_nao = devm_kmalloc(&pdev->dev,
> +							 size, GFP_KERNEL);
> +	if (!(dramc_dev_ptr->dramc_chn_base_nao))
> +		return -ENOMEM;
> +	dramc_dev_ptr->ddrphy_chn_base_ao = devm_kmalloc(&pdev->dev,
> +							 size, GFP_KERNEL);
> +	if (!(dramc_dev_ptr->ddrphy_chn_base_ao))
> +		return -ENOMEM;
> +	dramc_dev_ptr->ddrphy_chn_base_nao = devm_kmalloc(&pdev->dev,
> +							  size, GFP_KERNEL);
> +	if (!(dramc_dev_ptr->ddrphy_chn_base_nao))
> +		return -ENOMEM;
> +
> +	for (i = 0; i < dramc_dev_ptr->support_ch_cnt; i++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		dramc_dev_ptr->dramc_chn_base_ao[i] =
> +			devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(dramc_dev_ptr->dramc_chn_base_ao[i])) {
> +			pr_info("%s: unable to map ch%d DRAMC AO base\n",
> +				__func__, i);
> +			return -EINVAL;
> +		}
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM,
> +					    i + dramc_dev_ptr->support_ch_cnt);
> +		dramc_dev_ptr->dramc_chn_base_nao[i] =
> +			devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(dramc_dev_ptr->dramc_chn_base_nao[i])) {
> +			pr_info("%s: unable to map ch%d DRAMC NAO base\n",
> +				__func__, i);
> +			return -EINVAL;
> +		}
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM,
> +					    i + dramc_dev_ptr->support_ch_cnt * 2);
> +		dramc_dev_ptr->ddrphy_chn_base_ao[i] =
> +			devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_ao[i])) {
> +			pr_info("%s: unable to map ch%d DDRPHY AO base\n",
> +				__func__, i);
> +			return -EINVAL;
> +		}
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM,
> +					    i + dramc_dev_ptr->support_ch_cnt * 3);
> +		dramc_dev_ptr->ddrphy_chn_base_nao[i] =
> +			devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_nao[i])) {
> +			pr_info("%s: unable to map ch%d DDRPHY NAO base\n",
> +				__func__, i);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ret = of_property_read_u32(dramc_node, "fmeter-version", &fmeter_version);

This is something for platform data, not devicetree.

> +	if (ret) {
> +		pr_info("%s: get fmeter_version fail\n", __func__);
> +		return -EINVAL;
> +	}
> +	pr_info("%s: fmeter_version(%d)\n", __func__, fmeter_version);
> +
> +	if (fmeter_version == 1) {
> +		dramc_dev_ptr->fmeter_dev_ptr = devm_kmalloc(&pdev->dev,
> +							     sizeof(struct fmeter_dev_t),
> +							     GFP_KERNEL);
> +		if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
> +			pr_info("%s: memory  alloc fail\n", __func__);
> +			return -ENOMEM;
> +		}
> +		ret = fmeter_init(pdev, dramc_dev_ptr->fmeter_dev_ptr, fmeter_version);
> +		if (ret) {
> +			pr_info("%s: fmeter_init fail\n", __func__);
> +			return -EINVAL;
> +		}
> +	} else {
> +		dramc_dev_ptr->fmeter_dev_ptr = NULL;
> +	}
> +	ret = driver_create_file(pdev->dev.driver, &driver_attr_dram_data_rate);
> +	if (ret) {
> +		pr_info("%s: fail to create dram_data_rate sysfs\n", __func__);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, dramc_dev_ptr);
> +	pr_info("%s: DRAM data rate = %d\n", __func__,
> +		mtk_dramc_get_data_rate());
> +
> +	return ret;
> +}
> +
> +static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
> +{
> +	struct fmeter_dev_t *fmeter_dev_ptr =
> +		(struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> +	unsigned int shu_lv_val;
> +	unsigned int pll_id_val;
> +	unsigned int sdmpcw_val;
> +	unsigned int posdiv_val;
> +	unsigned int ckdiv4_val;
> +	unsigned int offset;
> +	unsigned int vco_freq;
> +	unsigned int fbksel;
> +	unsigned int dqsopen;
> +	unsigned int async_ca;
> +	unsigned int dq_ser_mode;
> +
> +	shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> +		fmeter_dev_ptr->shu_lv.offset) &
> +		fmeter_dev_ptr->shu_lv.mask) >>
> +		fmeter_dev_ptr->shu_lv.shift;
> +
> +	pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> +		fmeter_dev_ptr->pll_id.offset) &
> +		fmeter_dev_ptr->pll_id.mask) >>
> +		fmeter_dev_ptr->pll_id.shift;
> +
> +	offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
> +		fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
> +		fmeter_dev_ptr->posdiv[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
> +		fmeter_dev_ptr->fbksel[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
> +		fmeter_dev_ptr->dqsopen[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
> +		fmeter_dev_ptr->async_ca[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
> +		fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
> +	ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3: DIV16
> +
> +	posdiv_val &= ~(0x4);
> +
> +	vco_freq = ((fmeter_dev_ptr->crystal_freq) *
> +		(sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
> +		<< fbksel;

That vco_freq calculation is seriously unreadable :\

> +
> +	if ((dqsopen == 1) && (async_ca == 1))
> +		vco_freq >>= 1;
> +
> +	return vco_freq;
> +}
> +
> +/*
> + * mtk_dramc_get_data_rate - calculate DRAM data rate
> + *
> + * Returns DRAM data rate (MB/s)
> + */
> +unsigned int mtk_dramc_get_data_rate(void)
> +{
> +	struct dramc_dev_t *dramc_dev_ptr;
> +	struct fmeter_dev_t *fmeter_dev_ptr;
> +
> +	if (!dramc_pdev)
> +		return 0;
> +
> +	dramc_dev_ptr =
> +		(struct dramc_dev_t *)platform_get_drvdata(dramc_pdev);
> +
> +	fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> +	if (!fmeter_dev_ptr)
> +		return 0;
> +
> +	if (fmeter_dev_ptr->version == 1)
> +		return fmeter_v1(dramc_dev_ptr);
> +	return 0;
> +}
> +EXPORT_SYMBOL(mtk_dramc_get_data_rate);

Not sure about mtk_dramc_get_data_rate().

Please show an example of how you're actually using it in the external driver,
as there are many ways to expose this value.

> +
> +static int dramc_remove(struct platform_device *pdev)
> +{
> +	dramc_pdev = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dramc_of_ids[] = {
> +	{.compatible = "mediatek,common-dramc",},

{ .compatible = "mediatek,something" },
{ /* sentinel */ }

> +	{}
> +};
> +
> +static struct platform_driver dramc_drv = {
> +	.probe = dramc_probe,
> +	.remove = dramc_remove,
> +	.driver = {
> +		.name = "dramc_drv",
> +		.owner = THIS_MODULE,
> +		.of_match_table = dramc_of_ids,
> +	},
> +};

module_platform_driver(...)

> +
> +static int __init dramc_drv_init(void)

...so that function is not needed.

> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&dramc_drv);
> +	if (ret) {
> +		pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +module_init(dramc_drv_init);
> +
> +MODULE_AUTHOR("Mediatek Corporation");

MODULE_AUTHOR("Author Name <your.email@mediatek.com>");

> +MODULE_DESCRIPTION("MediaTek DRAMC Driver");

MODULE_DESCRIPTION("MediaTek DRAM Controller driver");



...and I have a hunch that this series will need many versions before the code
quality bar reaches a nice level...

Cheers,
Angelo

> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Krzysztof Kozlowski]

On 12/12/2024 09:59, Crystal Guo wrote:
> Add devicetree binding for mediatek common-dramc driver.
> 
> The DRAM controller of MediaTek SoC provides an interface to
> get the current data rate of DRAM.

Bindings are before users.



A nit, subject: drop second/last, redundant "dt-bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>  .../mediatek,common-dramc.yaml                | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> new file mode 100644
> index 000000000000..c9e608c7f183
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +# Copyright (c) 2024 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Common DRAMC (DRAM Controller)

Common? Is this a real thing? Please describe the hardware.

> +
> +maintainers:
> +  - Crystal Guo <crystal.guo@mediatek.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The DRAM controller of MediaTek SoC provides an interface to
> +  get the current data rate of DRAM.

So not common here?

> +
> +properties:
> +  compatible:
> +    const: mediatek,common-dramc

This has to be SoC.

> +
> +  reg:
> +    minItems: 9

Why this is flexible?

> +    items:
> +      - description: DRAMC_AO_CHA_BASE
> +      - description: DRAMC_AO_CHB_BASE
> +      - description: DRAMC_AO_CHC_BASE
> +      - description: DRAMC_AO_CHD_BASE
> +      - description: DRAMC_NAO_CHA_BASE
> +      - description: DRAMC_NAO_CHB_BASE
> +      - description: DRAMC_NAO_CHC_BASE
> +      - description: DRAMC_NAO_CHD_BASE
> +      - description: DDRPHY_AO_CHA_BASE
> +      - description: DDRPHY_AO_CHB_BASE
> +      - description: DDRPHY_AO_CHC_BASE
> +      - description: DDRPHY_AO_CHD_BASE
> +      - description: DDRPHY_NAO_CHA_BASE
> +      - description: DDRPHY_NAO_CHB_BASE
> +      - description: DDRPHY_NAO_CHC_BASE
> +      - description: DDRPHY_NAO_CHD_BASE
> +      - description: SLEEP_BASE

Don't use some defines. Look at other bindings how they describe items.

> +
> +  support-ch-cnt:

Nope

> +    maxItems: 1
> +
> +  fmeter-version:
> +    maxItems: 1
> +    description:
> +      Fmeter version for calculating dram data rate
> +
> +  crystal-freq:
> +    maxItems: 1
> +    description:
> +      Reference clock rate in MHz
> +
> +  shu-of:
> +    maxItems: 1
> +
> +  pll-id: true
> +  shu-lv: true
> +  sdmpcw: true
> +  posdiv: true
> +  fbksel: true
> +  dqsopen: true
> +  async-ca: true
> +  dq-ser-mode: true


This binding is terrible. Was not tested and does not follow any
guidelines. Please read example schema and writing bindings document.
You can also read slides from my talks...


> +
> +required:
> +  - compatible
> +  - reg
> +  - support-ch-cnt
> +  - fmeter-version
> +  - crystal-freq
> +  - pll-id
> +  - shu-lv
> +  - shu-of
> +  - sdmpcw
> +  - posdiv
> +  - fbksel
> +  - dqsopen
> +  - async-ca
> +  - dq-ser-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        dramc: dramc@10230000 {

memory-controller@
and drop unused label.

Best regards,
Krzysztof

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[Krzysztof Kozlowski]

On 12/12/2024 09:59, Crystal Guo wrote:
> Add MediaTek DRAMC driver to provide an interface that can
> obtain current DDR data rate.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>  drivers/memory/Kconfig                 |   1 +
>  drivers/memory/Makefile                |   1 +
>  drivers/memory/mediatek/Kconfig        |  21 ++
>  drivers/memory/mediatek/Makefile       |   2 +
>  drivers/memory/mediatek/mtk-dramc.c    | 325 +++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
>  6 files changed, 391 insertions(+)
>  create mode 100644 drivers/memory/mediatek/Kconfig
>  create mode 100644 drivers/memory/mediatek/Makefile
>  create mode 100644 drivers/memory/mediatek/mtk-dramc.c
>  create mode 100644 include/linux/soc/mediatek/mtk-dramc.h
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c82d8d8a16ea..b1698549ff81 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -227,5 +227,6 @@ config STM32_FMC2_EBI
>  
>  source "drivers/memory/samsung/Kconfig"
>  source "drivers/memory/tegra/Kconfig"
> +source "drivers/memory/mediatek/Kconfig"
>  
>  endif
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index d2e6ca9abbe0..cf1091449d2e 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)	+= stm32-fmc2-ebi.o
>  
>  obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
>  obj-$(CONFIG_TEGRA_MC)		+= tegra/
> +obj-$(CONFIG_HAVE_MTK_MC)	+= mediatek/

What is HAVE? Why this is not just MTK_MC or ARCH_MEDIATEK?

>  obj-$(CONFIG_TI_EMIF_SRAM)	+= ti-emif-sram.o
>  obj-$(CONFIG_FPGA_DFL_EMIF)	+= dfl-emif.o
>  
> diff --git a/drivers/memory/mediatek/Kconfig b/drivers/memory/mediatek/Kconfig
> new file mode 100644
> index 000000000000..00764cdb157e
> --- /dev/null
> +++ b/drivers/memory/mediatek/Kconfig
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config HAVE_MTK_MC
> +	bool "MediaTek Memory Controller support"
> +	help
> +	  This option allows to enable MediaTek memory controller drivers,
> +	  which may include controllers for DRAM or others.
> +	  Select Y here if you need support for MediaTek memory controller.
> +	  If you don't need, select N.
> +
> +if HAVE_MTK_MC
> +
> +config MTK_DRAMC
> +	tristate "MediaTek DRAMC driver"
> +	depends on HAVE_MTK_MC

Feels useless, isn't it part of if?

> +	help
> +	  This option selects the MediaTek DRAMC driver, which provides
> +	  an interface for reporting the current data rate of DRAM.
> +	  Select Y here if you need support for the MediaTek DRAMC driver.
> +	  If you don't need, select N.
> +
> +endif
> diff --git a/drivers/memory/mediatek/Makefile b/drivers/memory/mediatek/Makefile
> new file mode 100644
> index 000000000000..a1395fc55b41
> --- /dev/null
> +++ b/drivers/memory/mediatek/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_MTK_DRAMC)		+= mtk-dramc.o
> diff --git a/drivers/memory/mediatek/mtk-dramc.c b/drivers/memory/mediatek/mtk-dramc.c
> new file mode 100644
> index 000000000000..9c2c8e187a4a
> --- /dev/null
> +++ b/drivers/memory/mediatek/mtk-dramc.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 MediaTek Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/printk.h>
> +#include <linux/io.h>
> +#include <linux/soc/mediatek/mtk-dramc.h>
> +
> +static struct platform_device *dramc_pdev;
> +static struct platform_driver dramc_drv;

Drop both. Don't write singleton code.

...

> +		if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
> +			pr_info("%s: memory  alloc fail\n", __func__);
> +			return -ENOMEM;
> +		}
> +		ret = fmeter_init(pdev, dramc_dev_ptr->fmeter_dev_ptr, fmeter_version);
> +		if (ret) {
> +			pr_info("%s: fmeter_init fail\n", __func__);
> +			return -EINVAL;
> +		}
> +	} else {
> +		dramc_dev_ptr->fmeter_dev_ptr = NULL;
> +	}
> +	ret = driver_create_file(pdev->dev.driver, &driver_attr_dram_data_rate);
> +	if (ret) {
> +		pr_info("%s: fail to create dram_data_rate sysfs\n", __func__);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, dramc_dev_ptr);
> +	pr_info("%s: DRAM data rate = %d\n", __func__,
> +		mtk_dramc_get_data_rate());


This is terible coding style. Driver is in very poor shape.

Please perform extensive internal review in Mediatek to avoid sending
very poor quality contributions. This uses community resources in unfair
way. Few more comments, but all this code needs extensive rework and I
am not going to explain you basics of Linux Coding style, if you do not
want to take existing code as learning examples. Work with your
collegagues on that.


> +
> +	return ret;
> +}
> +
> +static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
> +{
> +	struct fmeter_dev_t *fmeter_dev_ptr =
> +		(struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> +	unsigned int shu_lv_val;
> +	unsigned int pll_id_val;
> +	unsigned int sdmpcw_val;
> +	unsigned int posdiv_val;
> +	unsigned int ckdiv4_val;
> +	unsigned int offset;
> +	unsigned int vco_freq;
> +	unsigned int fbksel;
> +	unsigned int dqsopen;
> +	unsigned int async_ca;
> +	unsigned int dq_ser_mode;
> +
> +	shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> +		fmeter_dev_ptr->shu_lv.offset) &
> +		fmeter_dev_ptr->shu_lv.mask) >>
> +		fmeter_dev_ptr->shu_lv.shift;
> +
> +	pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> +		fmeter_dev_ptr->pll_id.offset) &
> +		fmeter_dev_ptr->pll_id.mask) >>
> +		fmeter_dev_ptr->pll_id.shift;
> +
> +	offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
> +		fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
> +		fmeter_dev_ptr->posdiv[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
> +		fmeter_dev_ptr->fbksel[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
> +		fmeter_dev_ptr->dqsopen[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
> +		fmeter_dev_ptr->async_ca[pll_id_val].shift;
> +
> +	offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
> +		fmeter_dev_ptr->shu_of * shu_lv_val;
> +	dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] + offset) &
> +		fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
> +		fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
> +	ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3: DIV16
> +
> +	posdiv_val &= ~(0x4);
> +
> +	vco_freq = ((fmeter_dev_ptr->crystal_freq) *
> +		(sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
> +		<< fbksel;
> +
> +	if ((dqsopen == 1) && (async_ca == 1))
> +		vco_freq >>= 1;
> +
> +	return vco_freq;
> +}
> +
> +/*
> + * mtk_dramc_get_data_rate - calculate DRAM data rate
> + *
> + * Returns DRAM data rate (MB/s)
> + */
> +unsigned int mtk_dramc_get_data_rate(void)
> +{
> +	struct dramc_dev_t *dramc_dev_ptr;
> +	struct fmeter_dev_t *fmeter_dev_ptr;
> +
> +	if (!dramc_pdev)
> +		return 0;
> +
> +	dramc_dev_ptr =
> +		(struct dramc_dev_t *)platform_get_drvdata(dramc_pdev);
> +
> +	fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> +	if (!fmeter_dev_ptr)
> +		return 0;
> +
> +	if (fmeter_dev_ptr->version == 1)
> +		return fmeter_v1(dramc_dev_ptr);
> +	return 0;
> +}
> +EXPORT_SYMBOL(mtk_dramc_get_data_rate);

NAK, not used.

> +
> +static int dramc_remove(struct platform_device *pdev)
> +{
> +	dramc_pdev = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dramc_of_ids[] = {
> +	{.compatible = "mediatek,common-dramc",},
> +	{}
> +};
> +
> +static struct platform_driver dramc_drv = {
> +	.probe = dramc_probe,
> +	.remove = dramc_remove,
> +	.driver = {
> +		.name = "dramc_drv",
> +		.owner = THIS_MODULE,

NAK. You sent us 10 year old code, which means you replicated all the
bugs, issues and old-coding styles existing 10 years ago.

Start *FROM SCRATCH* from the newest driver, so you won't send us same
junk we already fixed during last 10 years.


> +		.of_match_table = dramc_of_ids,
> +	},
> +};
> +
> +static int __init dramc_drv_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&dramc_drv);
> +	if (ret) {
> +		pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +module_init(dramc_drv_init);

No, just module_platform_driver

> +
> +MODULE_AUTHOR("Mediatek Corporation");
> +MODULE_DESCRIPTION("MediaTek DRAMC Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/soc/mediatek/mtk-dramc.h b/include/linux/soc/mediatek/mtk-dramc.h
> new file mode 100644
> index 000000000000..95e7dbfe7d0e
> --- /dev/null
> +++ b/include/linux/soc/mediatek/mtk-dramc.h

Move everything into driver.

> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_DRAMC_H__
> +#define __MTK_DRAMC_H__
> +
> +struct reg_ctrl_t {
> +	unsigned int offset;
> +	unsigned int mask;
> +	unsigned int shift;
> +};
> +
> +struct fmeter_dev_t {
> +	unsigned int version;
> +	unsigned int crystal_freq;
> +	unsigned int shu_of;
> +	struct reg_ctrl_t shu_lv;
> +	struct reg_ctrl_t pll_id;
> +	struct reg_ctrl_t sdmpcw[2];
> +	struct reg_ctrl_t posdiv[2];
> +	struct reg_ctrl_t fbksel[2];
> +	struct reg_ctrl_t dqsopen[2];
> +	struct reg_ctrl_t async_ca[2];
> +	struct reg_ctrl_t dq_ser_mode[2];
> +};
> +
> +struct dramc_dev_t {
> +	unsigned int support_ch_cnt;
> +	void __iomem **dramc_chn_base_ao;
> +	void __iomem **dramc_chn_base_nao;
> +	void __iomem **ddrphy_chn_base_ao;
> +	void __iomem **ddrphy_chn_base_nao;
> +	void __iomem *sleep_base;
> +	void *fmeter_dev_ptr;
> +};
> +
> +unsigned int mtk_dramc_get_data_rate(void);


Drop, unused.

> +
> +#endif /* __MTK_DRAMC_H__ */


Best regards,
Krzysztof

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[kernel test robot]

Hi Crystal,

kernel test robot noticed the following build errors:

[auto build test ERROR on krzk-mem-ctrl/for-next]
[also build test ERROR on robh/for-next krzk-dt/for-next linus/master v6.13-rc3 next-20241220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Crystal-Guo/memory-mediatek-Add-an-interface-to-get-current-DDR-data-rate/20241212-170306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git for-next
patch link:    https://lore.kernel.org/r/20241212090029.13692-2-crystal.guo%40mediatek.com
patch subject: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241221/202412210955.FvO0Pee3-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241221/202412210955.FvO0Pee3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412210955.FvO0Pee3-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/memory/mediatek/mtk-dramc.c:300:19: error: initialization of 'void (*)(struct platform_device *)' from incompatible pointer type 'int (*)(struct platform_device *)' [-Wincompatible-pointer-types]
     300 |         .remove = dramc_remove,
         |                   ^~~~~~~~~~~~
   drivers/memory/mediatek/mtk-dramc.c:300:19: note: (near initialization for 'dramc_drv.<anonymous>.remove')


vim +300 drivers/memory/mediatek/mtk-dramc.c

   297	
   298	static struct platform_driver dramc_drv = {
   299		.probe = dramc_probe,
 > 300		.remove = dramc_remove,
   301		.driver = {
   302			.name = "dramc_drv",
   303			.owner = THIS_MODULE,
   304			.of_match_table = dramc_of_ids,
   305		},
   306	};
   307	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[kernel test robot]

Hi Crystal,

kernel test robot noticed the following build errors:

[auto build test ERROR on krzk-mem-ctrl/for-next]
[also build test ERROR on robh/for-next krzk-dt/for-next linus/master v6.13-rc3 next-20241220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Crystal-Guo/memory-mediatek-Add-an-interface-to-get-current-DDR-data-rate/20241212-170306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git for-next
patch link:    https://lore.kernel.org/r/20241212090029.13692-2-crystal.guo%40mediatek.com
patch subject: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20241221/202412211805.ulwSEmZa-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 9daf10ff8f29ba3a88a105aaa9d2379c21b77d35)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241221/202412211805.ulwSEmZa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412211805.ulwSEmZa-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/memory/mediatek/mtk-dramc.c:300:12: error: incompatible function pointer types initializing 'void (*)(struct platform_device *)' with an expression of type 'int (struct platform_device *)' [-Wincompatible-function-pointer-types]
     300 |         .remove = dramc_remove,
         |                   ^~~~~~~~~~~~
   1 error generated.


vim +300 drivers/memory/mediatek/mtk-dramc.c

   297	
   298	static struct platform_driver dramc_drv = {
   299		.probe = dramc_probe,
 > 300		.remove = dramc_remove,
   301		.driver = {
   302			.name = "dramc_drv",
   303			.owner = THIS_MODULE,
   304			.of_match_table = dramc_of_ids,
   305		},
   306	};
   307	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 11:46 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 12/12/24 09:59, Crystal Guo ha scritto:
> > Add MediaTek DRAMC driver to provide an interface that can
> > obtain current DDR data rate.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >   drivers/memory/Kconfig                 |   1 +
> >   drivers/memory/Makefile                |   1 +
> >   drivers/memory/mediatek/Kconfig        |  21 ++
> >   drivers/memory/mediatek/Makefile       |   2 +
> >   drivers/memory/mediatek/mtk-dramc.c    | 325
> > +++++++++++++++++++++++++
> >   include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
> >   6 files changed, 391 insertions(+)
> >   create mode 100644 drivers/memory/mediatek/Kconfig
> >   create mode 100644 drivers/memory/mediatek/Makefile
> >   create mode 100644 drivers/memory/mediatek/mtk-dramc.c
> >   create mode 100644 include/linux/soc/mediatek/mtk-dramc.h
> > 
> > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > index c82d8d8a16ea..b1698549ff81 100644
> > --- a/drivers/memory/Kconfig
> > +++ b/drivers/memory/Kconfig
> > @@ -227,5 +227,6 @@ config STM32_FMC2_EBI
> > 
> >   source "drivers/memory/samsung/Kconfig"
> >   source "drivers/memory/tegra/Kconfig"
> > +source "drivers/memory/mediatek/Kconfig"
> > 
> >   endif
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index d2e6ca9abbe0..cf1091449d2e 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)        += stm32-
> > fmc2-ebi.o
> > 
> >   obj-$(CONFIG_SAMSUNG_MC)    += samsung/
> >   obj-$(CONFIG_TEGRA_MC)              += tegra/
> > +obj-$(CONFIG_HAVE_MTK_MC)    += mediatek/
> >   obj-$(CONFIG_TI_EMIF_SRAM)  += ti-emif-sram.o
> >   obj-$(CONFIG_FPGA_DFL_EMIF) += dfl-emif.o
> > 
> > diff --git a/drivers/memory/mediatek/Kconfig
> > b/drivers/memory/mediatek/Kconfig
> > new file mode 100644
> > index 000000000000..00764cdb157e
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Kconfig
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config HAVE_MTK_MC
> > +     bool "MediaTek Memory Controller support"
> > +     help
> > +       This option allows to enable MediaTek memory controller
> > drivers,
> > +       which may include controllers for DRAM or others.
> > +       Select Y here if you need support for MediaTek memory
> > controller.
> > +       If you don't need, select N.
> > +
> > +if HAVE_MTK_MC
> > +
> > +config MTK_DRAMC
> > +     tristate "MediaTek DRAMC driver"
> > +     depends on HAVE_MTK_MC
> > +     help
> > +       This option selects the MediaTek DRAMC driver, which
> > provides
> > +       an interface for reporting the current data rate of DRAM.
> > +       Select Y here if you need support for the MediaTek DRAMC
> > driver.
> > +       If you don't need, select N.
> > +
> > +endif
> > diff --git a/drivers/memory/mediatek/Makefile
> > b/drivers/memory/mediatek/Makefile
> > new file mode 100644
> > index 000000000000..a1395fc55b41
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_MTK_DRAMC)              += mtk-dramc.o
> > diff --git a/drivers/memory/mediatek/mtk-dramc.c
> > b/drivers/memory/mediatek/mtk-dramc.c
> > new file mode 100644
> > index 000000000000..9c2c8e187a4a
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/mtk-dramc.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/printk.h>
> > +#include <linux/io.h>
> > +#include <linux/soc/mediatek/mtk-dramc.h>
> > +
> > +static struct platform_device *dramc_pdev;
> > +static struct platform_driver dramc_drv;
> 
> You don't need these globals.
> 
> > +
> > +static int fmeter_init(struct platform_device *pdev,
> > +                    struct fmeter_dev_t *fmeter_dev_ptr, unsigned
> > int fmeter_version)
> > +{
> > +     struct device_node *dramc_node = pdev->dev.of_node;
> > +     int ret;
> > +
> > +     ret = of_property_read_u32(dramc_node,
> > +                                "crystal-freq", &(fmeter_dev_ptr-
> > >crystal_freq));
> > +     ret |= of_property_read_u32(dramc_node,
> > +                                 "shu-of", &(fmeter_dev_ptr-
> > >shu_of));
> 
> ret |= x doesn't make sense.
> 
> ret = ...
> if (ret)
>         return ret;
> 
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "shu-lv", (unsigned int
> > *)&(fmeter_dev_ptr->shu_lv), 3);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "pll-id", (unsigned int
> > *)&(fmeter_dev_ptr->pll_id), 3);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "sdmpcw", (unsigned int
> > *)(fmeter_dev_ptr->sdmpcw), 6);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "posdiv", (unsigned int
> > *)(fmeter_dev_ptr->posdiv), 6);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "fbksel", (unsigned int
> > *)(fmeter_dev_ptr->fbksel), 6);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "dqsopen", (unsigned int
> > *)(fmeter_dev_ptr->dqsopen), 6);
> > +     if (fmeter_version == 1) {
> > +             fmeter_dev_ptr->version = 1;
> > +             ret |= of_property_read_u32_array(dramc_node,
> > +                     "async-ca", (unsigned int *)(fmeter_dev_ptr-
> > >async_ca), 6);
> > +             ret |= of_property_read_u32_array(dramc_node,
> > +                     "dq-ser-mode", (unsigned int
> > *)(fmeter_dev_ptr->dq_ser_mode), 6);
> > +     }
> > +     return ret;
> > +}
> > +
> > +static ssize_t dram_data_rate_show(struct device_driver *driver,
> > char *buf)
> > +{
> > +     return snprintf(buf, PAGE_SIZE, "DRAM data rate = %d\n",
> > +             mtk_dramc_get_data_rate());
> > +}
> > +
> > +static DRIVER_ATTR_RO(dram_data_rate);> +
> > +static int dramc_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *dramc_node = pdev->dev.of_node;
> > +     struct dramc_dev_t *dramc_dev_ptr;
> > +     unsigned int fmeter_version;
> > +     struct resource *res;
> > +     unsigned int i, size;
> > +     int ret;
> > +
> > +     pr_info("%s: module probe.\n", __func__);
> 
> This pr_info makes no sense. Please remove.
> 
> > +     dramc_pdev = pdev;
> > +     dramc_dev_ptr = devm_kmalloc(&pdev->dev,
> > +                                  sizeof(struct dramc_dev_t),
> > GFP_KERNEL);
> > +
> > +     if (!dramc_dev_ptr)
> > +             return -ENOMEM;
> > +
> > +     ret = of_property_read_u32(dramc_node,
> > +                                "support-ch-cnt", &dramc_dev_ptr-
> > >support_ch_cnt);
> > +     if (ret) {
> > +             pr_info("%s: get support_ch_cnt fail\n", __func__);
> 
> that's an error, and you want dev_err_probe().
> 
> > +             return -EINVAL;
> > +     }
> > +
> > +     dramc_dev_ptr->sleep_base = of_iomap(dramc_node,
> > +                                          dramc_dev_ptr-
> > >support_ch_cnt * 4);
> > +     if (IS_ERR(dramc_dev_ptr->sleep_base)) {
> > +             pr_info("%s: unable to map sleep base\n", __func__);
> 
> same, here and everywhere else
> 
> > +             return -EINVAL;
> > +     }
> > +
> > +     size = sizeof(phys_addr_t) * dramc_dev_ptr->support_ch_cnt;
> > +     dramc_dev_ptr->dramc_chn_base_ao = devm_kmalloc(&pdev->dev,
> > +                                                     size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->dramc_chn_base_ao))
> > +             return -ENOMEM;
> > +     dramc_dev_ptr->dramc_chn_base_nao = devm_kmalloc(&pdev->dev,
> > +                                                      size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->dramc_chn_base_nao))
> > +             return -ENOMEM;
> > +     dramc_dev_ptr->ddrphy_chn_base_ao = devm_kmalloc(&pdev->dev,
> > +                                                      size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->ddrphy_chn_base_ao))
> > +             return -ENOMEM;
> > +     dramc_dev_ptr->ddrphy_chn_base_nao = devm_kmalloc(&pdev->dev,
> > +                                                       size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->ddrphy_chn_base_nao))
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < dramc_dev_ptr->support_ch_cnt; i++) {
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > +             dramc_dev_ptr->dramc_chn_base_ao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->dramc_chn_base_ao[i])) {
> > +                     pr_info("%s: unable to map ch%d DRAMC AO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM,
> > +                                         i + dramc_dev_ptr-
> > >support_ch_cnt);
> > +             dramc_dev_ptr->dramc_chn_base_nao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->dramc_chn_base_nao[i])) {
> > +                     pr_info("%s: unable to map ch%d DRAMC NAO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM,
> > +                                         i + dramc_dev_ptr-
> > >support_ch_cnt * 2);
> > +             dramc_dev_ptr->ddrphy_chn_base_ao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_ao[i])) {
> > +                     pr_info("%s: unable to map ch%d DDRPHY AO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM,
> > +                                         i + dramc_dev_ptr-
> > >support_ch_cnt * 3);
> > +             dramc_dev_ptr->ddrphy_chn_base_nao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_nao[i])) {
> > +                     pr_info("%s: unable to map ch%d DDRPHY NAO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     ret = of_property_read_u32(dramc_node, "fmeter-version",
> > &fmeter_version);
> 
> This is something for platform data, not devicetree.
> 
> > +     if (ret) {
> > +             pr_info("%s: get fmeter_version fail\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +     pr_info("%s: fmeter_version(%d)\n", __func__,
> > fmeter_version);
> > +
> > +     if (fmeter_version == 1) {
> > +             dramc_dev_ptr->fmeter_dev_ptr = devm_kmalloc(&pdev-
> > >dev,
> > +                                                          sizeof(s
> > truct fmeter_dev_t),
> > +                                                          GFP_KERN
> > EL);
> > +             if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
> > +                     pr_info("%s: memory  alloc fail\n",
> > __func__);
> > +                     return -ENOMEM;
> > +             }
> > +             ret = fmeter_init(pdev, dramc_dev_ptr-
> > >fmeter_dev_ptr, fmeter_version);
> > +             if (ret) {
> > +                     pr_info("%s: fmeter_init fail\n", __func__);
> > +                     return -EINVAL;
> > +             }
> > +     } else {
> > +             dramc_dev_ptr->fmeter_dev_ptr = NULL;
> > +     }
> > +     ret = driver_create_file(pdev->dev.driver,
> > &driver_attr_dram_data_rate);
> > +     if (ret) {
> > +             pr_info("%s: fail to create dram_data_rate sysfs\n",
> > __func__);
> > +             return ret;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, dramc_dev_ptr);
> > +     pr_info("%s: DRAM data rate = %d\n", __func__,
> > +             mtk_dramc_get_data_rate());
> > +
> > +     return ret;
> > +}
> > +
> > +static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
> > +{
> > +     struct fmeter_dev_t *fmeter_dev_ptr =
> > +             (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> > +     unsigned int shu_lv_val;
> > +     unsigned int pll_id_val;
> > +     unsigned int sdmpcw_val;
> > +     unsigned int posdiv_val;
> > +     unsigned int ckdiv4_val;
> > +     unsigned int offset;
> > +     unsigned int vco_freq;
> > +     unsigned int fbksel;
> > +     unsigned int dqsopen;
> > +     unsigned int async_ca;
> > +     unsigned int dq_ser_mode;
> > +
> > +     shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->shu_lv.offset) &
> > +             fmeter_dev_ptr->shu_lv.mask) >>
> > +             fmeter_dev_ptr->shu_lv.shift;
> > +
> > +     pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->pll_id.offset) &
> > +             fmeter_dev_ptr->pll_id.mask) >>
> > +             fmeter_dev_ptr->pll_id.shift;
> > +
> > +     offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->posdiv[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->fbksel[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->async_ca[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
> > +     ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3:
> > DIV16
> > +
> > +     posdiv_val &= ~(0x4);
> > +
> > +     vco_freq = ((fmeter_dev_ptr->crystal_freq) *
> > +             (sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
> > +             << fbksel;
> 
> That vco_freq calculation is seriously unreadable :\
> 
> > +
> > +     if ((dqsopen == 1) && (async_ca == 1))
> > +             vco_freq >>= 1;
> > +
> > +     return vco_freq;
> > +}
> > +
> > +/*
> > + * mtk_dramc_get_data_rate - calculate DRAM data rate
> > + *
> > + * Returns DRAM data rate (MB/s)
> > + */
> > +unsigned int mtk_dramc_get_data_rate(void)
> > +{
> > +     struct dramc_dev_t *dramc_dev_ptr;
> > +     struct fmeter_dev_t *fmeter_dev_ptr;
> > +
> > +     if (!dramc_pdev)
> > +             return 0;
> > +
> > +     dramc_dev_ptr =
> > +             (struct dramc_dev_t
> > *)platform_get_drvdata(dramc_pdev);
> > +
> > +     fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr-
> > >fmeter_dev_ptr;
> > +     if (!fmeter_dev_ptr)
> > +             return 0;
> > +
> > +     if (fmeter_dev_ptr->version == 1)
> > +             return fmeter_v1(dramc_dev_ptr);
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(mtk_dramc_get_data_rate);
> 
> Not sure about mtk_dramc_get_data_rate().
> 
> Please show an example of how you're actually using it in the
> external driver,
> as there are many ways to expose this value.
> 
> > +
> > +static int dramc_remove(struct platform_device *pdev)
> > +{
> > +     dramc_pdev = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id dramc_of_ids[] = {
> > +     {.compatible = "mediatek,common-dramc",},
> 
> { .compatible = "mediatek,something" },
> { /* sentinel */ }
> 
> > +     {}
> > +};
> > +
> > +static struct platform_driver dramc_drv = {
> > +     .probe = dramc_probe,
> > +     .remove = dramc_remove,
> > +     .driver = {
> > +             .name = "dramc_drv",
> > +             .owner = THIS_MODULE,
> > +             .of_match_table = dramc_of_ids,
> > +     },
> > +};
> 
> module_platform_driver(...)
> 
> > +
> > +static int __init dramc_drv_init(void)
> 
> ...so that function is not needed.
> 
> > +{
> > +     int ret;
> > +
> > +     ret = platform_driver_register(&dramc_drv);
> > +     if (ret) {
> > +             pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +module_init(dramc_drv_init);
> > +
> > +MODULE_AUTHOR("Mediatek Corporation");
> 
> MODULE_AUTHOR("Author Name <your.email@mediatek.com>");
> 
> > +MODULE_DESCRIPTION("MediaTek DRAMC Driver");
> 
> MODULE_DESCRIPTION("MediaTek DRAM Controller driver");
> 
> 
> 
> ...and I have a hunch that this series will need many versions before
> the code
> quality bar reaches a nice level...
> 
> Cheers,
> Angelo

Thanks for your review.
Following your suggestion, the v2 patch:
https://patchwork.kernel.org/patch/13964208 has been pushed.

Thanks
Crystal
> 
> > +MODULE_LICENSE("GPL");

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 12:02 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 12/12/2024 09:59, Crystal Guo wrote:
> > Add MediaTek DRAMC driver to provide an interface that can
> > obtain current DDR data rate.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >  drivers/memory/Kconfig                 |   1 +
> >  drivers/memory/Makefile                |   1 +
> >  drivers/memory/mediatek/Kconfig        |  21 ++
> >  drivers/memory/mediatek/Makefile       |   2 +
> >  drivers/memory/mediatek/mtk-dramc.c    | 325
> > +++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
> >  6 files changed, 391 insertions(+)
> >  create mode 100644 drivers/memory/mediatek/Kconfig
> >  create mode 100644 drivers/memory/mediatek/Makefile
> >  create mode 100644 drivers/memory/mediatek/mtk-dramc.c
> >  create mode 100644 include/linux/soc/mediatek/mtk-dramc.h
> > 
> > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > index c82d8d8a16ea..b1698549ff81 100644
> > --- a/drivers/memory/Kconfig
> > +++ b/drivers/memory/Kconfig
> > @@ -227,5 +227,6 @@ config STM32_FMC2_EBI
> > 
> >  source "drivers/memory/samsung/Kconfig"
> >  source "drivers/memory/tegra/Kconfig"
> > +source "drivers/memory/mediatek/Kconfig"
> > 
> >  endif
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index d2e6ca9abbe0..cf1091449d2e 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)        += stm32-
> > fmc2-ebi.o
> > 
> >  obj-$(CONFIG_SAMSUNG_MC)     += samsung/
> >  obj-$(CONFIG_TEGRA_MC)               += tegra/
> > +obj-$(CONFIG_HAVE_MTK_MC)    += mediatek/
> 
> What is HAVE? Why this is not just MTK_MC or ARCH_MEDIATEK?
> 
> >  obj-$(CONFIG_TI_EMIF_SRAM)   += ti-emif-sram.o
> >  obj-$(CONFIG_FPGA_DFL_EMIF)  += dfl-emif.o
> > 
> > diff --git a/drivers/memory/mediatek/Kconfig
> > b/drivers/memory/mediatek/Kconfig
> > new file mode 100644
> > index 000000000000..00764cdb157e
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Kconfig
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config HAVE_MTK_MC
> > +     bool "MediaTek Memory Controller support"
> > +     help
> > +       This option allows to enable MediaTek memory controller
> > drivers,
> > +       which may include controllers for DRAM or others.
> > +       Select Y here if you need support for MediaTek memory
> > controller.
> > +       If you don't need, select N.
> > +
> > +if HAVE_MTK_MC
> > +
> > +config MTK_DRAMC
> > +     tristate "MediaTek DRAMC driver"
> > +     depends on HAVE_MTK_MC
> 
> Feels useless, isn't it part of if?
> 
> > +     help
> > +       This option selects the MediaTek DRAMC driver, which
> > provides
> > +       an interface for reporting the current data rate of DRAM.
> > +       Select Y here if you need support for the MediaTek DRAMC
> > driver.
> > +       If you don't need, select N.
> > +
> > +endif
> > diff --git a/drivers/memory/mediatek/Makefile
> > b/drivers/memory/mediatek/Makefile
> > new file mode 100644
> > index 000000000000..a1395fc55b41
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_MTK_DRAMC)              += mtk-dramc.o
> > diff --git a/drivers/memory/mediatek/mtk-dramc.c
> > b/drivers/memory/mediatek/mtk-dramc.c
> > new file mode 100644
> > index 000000000000..9c2c8e187a4a
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/mtk-dramc.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/printk.h>
> > +#include <linux/io.h>
> > +#include <linux/soc/mediatek/mtk-dramc.h>
> > +
> > +static struct platform_device *dramc_pdev;
> > +static struct platform_driver dramc_drv;
> 
> Drop both. Don't write singleton code.
> 
> ...
> 
> > +             if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
> > +                     pr_info("%s: memory  alloc fail\n",
> > __func__);
> > +                     return -ENOMEM;
> > +             }
> > +             ret = fmeter_init(pdev, dramc_dev_ptr-
> > >fmeter_dev_ptr, fmeter_version);
> > +             if (ret) {
> > +                     pr_info("%s: fmeter_init fail\n", __func__);
> > +                     return -EINVAL;
> > +             }
> > +     } else {
> > +             dramc_dev_ptr->fmeter_dev_ptr = NULL;
> > +     }
> > +     ret = driver_create_file(pdev->dev.driver,
> > &driver_attr_dram_data_rate);
> > +     if (ret) {
> > +             pr_info("%s: fail to create dram_data_rate sysfs\n",
> > __func__);
> > +             return ret;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, dramc_dev_ptr);
> > +     pr_info("%s: DRAM data rate = %d\n", __func__,
> > +             mtk_dramc_get_data_rate());
> 
> 
> This is terible coding style. Driver is in very poor shape.
> 
> Please perform extensive internal review in Mediatek to avoid sending
> very poor quality contributions. This uses community resources in
> unfair
> way. Few more comments, but all this code needs extensive rework and
> I
> am not going to explain you basics of Linux Coding style, if you do
> not
> want to take existing code as learning examples. Work with your
> collegagues on that.
> 
> 
> > +
> > +     return ret;
> > +}
> > +
> > +static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
> > +{
> > +     struct fmeter_dev_t *fmeter_dev_ptr =
> > +             (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> > +     unsigned int shu_lv_val;
> > +     unsigned int pll_id_val;
> > +     unsigned int sdmpcw_val;
> > +     unsigned int posdiv_val;
> > +     unsigned int ckdiv4_val;
> > +     unsigned int offset;
> > +     unsigned int vco_freq;
> > +     unsigned int fbksel;
> > +     unsigned int dqsopen;
> > +     unsigned int async_ca;
> > +     unsigned int dq_ser_mode;
> > +
> > +     shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->shu_lv.offset) &
> > +             fmeter_dev_ptr->shu_lv.mask) >>
> > +             fmeter_dev_ptr->shu_lv.shift;
> > +
> > +     pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->pll_id.offset) &
> > +             fmeter_dev_ptr->pll_id.mask) >>
> > +             fmeter_dev_ptr->pll_id.shift;
> > +
> > +     offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->posdiv[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->fbksel[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->async_ca[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
> > +     ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3:
> > DIV16
> > +
> > +     posdiv_val &= ~(0x4);
> > +
> > +     vco_freq = ((fmeter_dev_ptr->crystal_freq) *
> > +             (sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
> > +             << fbksel;
> > +
> > +     if ((dqsopen == 1) && (async_ca == 1))
> > +             vco_freq >>= 1;
> > +
> > +     return vco_freq;
> > +}
> > +
> > +/*
> > + * mtk_dramc_get_data_rate - calculate DRAM data rate
> > + *
> > + * Returns DRAM data rate (MB/s)
> > + */
> > +unsigned int mtk_dramc_get_data_rate(void)
> > +{
> > +     struct dramc_dev_t *dramc_dev_ptr;
> > +     struct fmeter_dev_t *fmeter_dev_ptr;
> > +
> > +     if (!dramc_pdev)
> > +             return 0;
> > +
> > +     dramc_dev_ptr =
> > +             (struct dramc_dev_t
> > *)platform_get_drvdata(dramc_pdev);
> > +
> > +     fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr-
> > >fmeter_dev_ptr;
> > +     if (!fmeter_dev_ptr)
> > +             return 0;
> > +
> > +     if (fmeter_dev_ptr->version == 1)
> > +             return fmeter_v1(dramc_dev_ptr);
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(mtk_dramc_get_data_rate);
> 
> NAK, not used.
> 
> > +
> > +static int dramc_remove(struct platform_device *pdev)
> > +{
> > +     dramc_pdev = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id dramc_of_ids[] = {
> > +     {.compatible = "mediatek,common-dramc",},
> > +     {}
> > +};
> > +
> > +static struct platform_driver dramc_drv = {
> > +     .probe = dramc_probe,
> > +     .remove = dramc_remove,
> > +     .driver = {
> > +             .name = "dramc_drv",
> > +             .owner = THIS_MODULE,
> 
> NAK. You sent us 10 year old code, which means you replicated all the
> bugs, issues and old-coding styles existing 10 years ago.
> 
> Start *FROM SCRATCH* from the newest driver, so you won't send us
> same
> junk we already fixed during last 10 years.
> 
> 
> > +             .of_match_table = dramc_of_ids,
> > +     },
> > +};
> > +
> > +static int __init dramc_drv_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = platform_driver_register(&dramc_drv);
> > +     if (ret) {
> > +             pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +module_init(dramc_drv_init);
> 
> No, just module_platform_driver
> 
> > +
> > +MODULE_AUTHOR("Mediatek Corporation");
> > +MODULE_DESCRIPTION("MediaTek DRAMC Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/soc/mediatek/mtk-dramc.h
> > b/include/linux/soc/mediatek/mtk-dramc.h
> > new file mode 100644
> > index 000000000000..95e7dbfe7d0e
> > --- /dev/null
> > +++ b/include/linux/soc/mediatek/mtk-dramc.h
> 
> Move everything into driver.
> 
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_DRAMC_H__
> > +#define __MTK_DRAMC_H__
> > +
> > +struct reg_ctrl_t {
> > +     unsigned int offset;
> > +     unsigned int mask;
> > +     unsigned int shift;
> > +};
> > +
> > +struct fmeter_dev_t {
> > +     unsigned int version;
> > +     unsigned int crystal_freq;
> > +     unsigned int shu_of;
> > +     struct reg_ctrl_t shu_lv;
> > +     struct reg_ctrl_t pll_id;
> > +     struct reg_ctrl_t sdmpcw[2];
> > +     struct reg_ctrl_t posdiv[2];
> > +     struct reg_ctrl_t fbksel[2];
> > +     struct reg_ctrl_t dqsopen[2];
> > +     struct reg_ctrl_t async_ca[2];
> > +     struct reg_ctrl_t dq_ser_mode[2];
> > +};
> > +
> > +struct dramc_dev_t {
> > +     unsigned int support_ch_cnt;
> > +     void __iomem **dramc_chn_base_ao;
> > +     void __iomem **dramc_chn_base_nao;
> > +     void __iomem **ddrphy_chn_base_ao;
> > +     void __iomem **ddrphy_chn_base_nao;
> > +     void __iomem *sleep_base;
> > +     void *fmeter_dev_ptr;
> > +};
> > +
> > +unsigned int mtk_dramc_get_data_rate(void);
> 
> 
> Drop, unused.
> 
> > +
> > +#endif /* __MTK_DRAMC_H__ */
> 
> 
> Best regards,
> Krzysztof

Sorry for the inconvenience caused to you.
And thanks for your suggestion, I have referred to the above
suggestions and pushed the v2 patch again:
https://patchwork.kernel.org/patch/13964208

Thanks
Crystal

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 04:27 -0600, Rob Herring (Arm) wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, 12 Dec 2024 16:59:48 +0800, Crystal Guo wrote:
> > Add devicetree binding for mediatek common-dramc driver.
> > 
> > The DRAM controller of MediaTek SoC provides an interface to
> > get the current data rate of DRAM.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >  .../mediatek,common-dramc.yaml                | 129
> > ++++++++++++++++++
> >  1 file changed, 129 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: support-ch-cnt: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: fmeter-version: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: crystal-freq: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: shu-of: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: pll-id: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: shu-lv: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: sdmpcw: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: posdiv: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: fbksel: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: dqsopen: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: async-ca: missing type
> definition
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/memory-
> controllers/mediatek,common-dramc.yaml: dq-ser-mode: missing type
> definition
> 
> doc reference errors (make refcheckdocs):
> 
> See 
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241212090029.13692-3-crystal.guo@mediatek.com
> 
> The base for the series is generally the latest rc1. A different
> dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself.
> Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up
> checking
> your schema. However, it must be unset to test all examples with your
> schema.
> 

Thank you for the reminder.
Fix the build error on v2:https://patchwork.kernel.org/patch/13964209

Thanks
Crystal

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 11:27 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 12/12/24 09:59, Crystal Guo ha scritto:
> > Add devicetree binding for mediatek common-dramc driver.
> > 
> > The DRAM controller of MediaTek SoC provides an interface to
> > get the current data rate of DRAM.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >   .../mediatek,common-dramc.yaml                | 129
> > ++++++++++++++++++
> >   1 file changed, 129 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > new file mode 100644
> > index 000000000000..c9e608c7f183
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +# Copyright (c) 2024 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml*__;Iw!!CTRNKA9wMg0ARbw!hkObFeP7J-uvNSyDsepqqdH_jZQcf9NQ1knBRGY1ODpH6FoZyBzL1x5rEIWPppp1wNmLdo41PQTZM4ulMP1Qg7wr9PwFEdeN$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!hkObFeP7J-uvNSyDsepqqdH_jZQcf9NQ1knBRGY1ODpH6FoZyBzL1x5rEIWPppp1wNmLdo41PQTZM4ulMP1Qg7wr9FN7m95w$
> > +
> > +title: MediaTek Common DRAMC (DRAM Controller)
> 
> MediaTek DRAM Controller (DRAMC)
> 
> > +
> > +maintainers:
> > +  - Crystal Guo <crystal.guo@mediatek.com>
> > +
> > +description: |
> > +  The DRAM controller of MediaTek SoC provides an interface to
> > +  get the current data rate of DRAM.
> 
> No, the DRAM Controller does much more than just that.
> 
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,common-dramc
> 
> Absolutely no! Compatibles are per-soc.
> 
> mediatek,mt8186-dramc
> mediatek,mt8188-dramc
> mediatek,mt8195-dramc
> 
> etc
> 
> > +
> > +  reg:
> > +    minItems: 9
> > +    items:
> > +      - description: DRAMC_AO_CHA_BASE
> > +      - description: DRAMC_AO_CHB_BASE
> > +      - description: DRAMC_AO_CHC_BASE
> > +      - description: DRAMC_AO_CHD_BASE
> 
> All those channels are sequential in AO->NAO, in the sense that
> every channel is:
> 
> CH0     AO: 0x10230000   len: 0x4000
> CH0    NAO: 0x10234000   len: 0x2000
> CH0 PHY_AO: 0x10236000   len: 0x2000
> CH0 PHY_AO: 0x10238000   len: 0x2000
> 
> So the reg can be simplified as
> 
> minItems: 4
> items:
>    - description: DRAM Controller Channel 0
>    - description: DRAM Controller Channel 1
>    - description: DRAM Controller Channel 2
>    - description: DRAM Controller Channel 3
> 
> 
> > +      - description: DRAMC_NAO_CHA_BASE
> > +      - description: DRAMC_NAO_CHB_BASE
> > +      - description: DRAMC_NAO_CHC_BASE
> > +      - description: DRAMC_NAO_CHD_BASE
> > +      - description: DDRPHY_AO_CHA_BASE
> > +      - description: DDRPHY_AO_CHB_BASE
> > +      - description: DDRPHY_AO_CHC_BASE
> > +      - description: DDRPHY_AO_CHD_BASE
> > +      - description: DDRPHY_NAO_CHA_BASE
> > +      - description: DDRPHY_NAO_CHB_BASE
> > +      - description: DDRPHY_NAO_CHC_BASE
> > +      - description: DDRPHY_NAO_CHD_BASE
> > +      - description: SLEEP_BASE
> 
> You're not using the SLEEP_BASE iospace, and that's not even really
> specific
> to the DRAM Controller. Drop it.
> 
> > +
> > +  support-ch-cnt:
> > +    maxItems: 1
> 
> Don't tell me that the DRAM Controller in MediaTek SoCs cannot see
> how many
> channels are actually occupied by a DRAM bank, because I will be
> really skeptical.
> 
> You can autodetect that in the driver, you don't need a DT property
> for that.
> 
> > +
> > +  fmeter-version:
> > +    maxItems: 1
> > +    description:
> > +      Fmeter version for calculating dram data rate
> 
> The Fmeter version is SoC-specific, you need platform data, not DT
> property.
> 
> > +
> > +  crystal-freq:
> > +    maxItems: 1
> > +    description:
> > +      Reference clock rate in MHz
> 
> Is this crystal an external component, or is it integrated into the
> SoC?
> 
> > +
> > +  shu-of:
> > +    maxItems: 1
> 
> There's no description, what is shu-of?
> 
> > +
> > +  pll-id: true
> > +  shu-lv: true
> > +  sdmpcw: true
> > +  posdiv: true
> > +  fbksel: true
> > +  dqsopen: true
> > +  async-ca: true
> > +  dq-ser-mode: true
> 
> Same for these ones, please describe them - but then remember: if
> those parameters
> are board-specific, they can stay here, otherwise those go in
> platform data.
> 
> Besides, I doubt that those are board specific.
> 
> Regards,
> Angelo
> 

Thanks for your review.
Based on the above suggestions, I have simplified the parameters that
need to be passed into the DTS.
- Only the base address need to pass through the DTS;
- "fmeter version" is placed in the platform data;
- The values of "pll-id", "shu-lv"..., can be directly read in the
driver through predefined register offsets and masks.

v2: https://patchwork.kernel.org/patch/13964209

Thanks
Crystal

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 11:58 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 12/12/2024 09:59, Crystal Guo wrote:
> > Add devicetree binding for mediatek common-dramc driver.
> > 
> > The DRAM controller of MediaTek SoC provides an interface to
> > get the current data rate of DRAM.
> 
> Bindings are before users.
> 
> 
> 
> A nit, subject: drop second/last, redundant "dt-bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> 
https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst*L18__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5DM7MWDx$
> 
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >  .../mediatek,common-dramc.yaml                | 129
> > ++++++++++++++++++
> >  1 file changed, 129 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > new file mode 100644
> > index 000000000000..c9e608c7f183
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +# Copyright (c) 2024 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5GD5-Mgk$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5AGE5Eci$
> > +
> > +title: MediaTek Common DRAMC (DRAM Controller)
> 
> Common? Is this a real thing? Please describe the hardware.
> 
> > +
> > +maintainers:
> > +  - Crystal Guo <crystal.guo@mediatek.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  The DRAM controller of MediaTek SoC provides an interface to
> > +  get the current data rate of DRAM.
> 
> So not common here?
> 
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,common-dramc
> 
> This has to be SoC.
> 
> > +
> > +  reg:
> > +    minItems: 9
> 
> Why this is flexible?
> 
> > +    items:
> > +      - description: DRAMC_AO_CHA_BASE
> > +      - description: DRAMC_AO_CHB_BASE
> > +      - description: DRAMC_AO_CHC_BASE
> > +      - description: DRAMC_AO_CHD_BASE
> > +      - description: DRAMC_NAO_CHA_BASE
> > +      - description: DRAMC_NAO_CHB_BASE
> > +      - description: DRAMC_NAO_CHC_BASE
> > +      - description: DRAMC_NAO_CHD_BASE
> > +      - description: DDRPHY_AO_CHA_BASE
> > +      - description: DDRPHY_AO_CHB_BASE
> > +      - description: DDRPHY_AO_CHC_BASE
> > +      - description: DDRPHY_AO_CHD_BASE
> > +      - description: DDRPHY_NAO_CHA_BASE
> > +      - description: DDRPHY_NAO_CHB_BASE
> > +      - description: DDRPHY_NAO_CHC_BASE
> > +      - description: DDRPHY_NAO_CHD_BASE
> > +      - description: SLEEP_BASE
> 
> Don't use some defines. Look at other bindings how they describe
> items.
> 
> > +
> > +  support-ch-cnt:
> 
> Nope
> 
> > +    maxItems: 1
> > +
> > +  fmeter-version:
> > +    maxItems: 1
> > +    description:
> > +      Fmeter version for calculating dram data rate
> > +
> > +  crystal-freq:
> > +    maxItems: 1
> > +    description:
> > +      Reference clock rate in MHz
> > +
> > +  shu-of:
> > +    maxItems: 1
> > +
> > +  pll-id: true
> > +  shu-lv: true
> > +  sdmpcw: true
> > +  posdiv: true
> > +  fbksel: true
> > +  dqsopen: true
> > +  async-ca: true
> > +  dq-ser-mode: true
> 
> 
> This binding is terrible. Was not tested and does not follow any
> guidelines. Please read example schema and writing bindings document.
> You can also read slides from my talks...
> 
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - support-ch-cnt
> > +  - fmeter-version
> > +  - crystal-freq
> > +  - pll-id
> > +  - shu-lv
> > +  - shu-of
> > +  - sdmpcw
> > +  - posdiv
> > +  - fbksel
> > +  - dqsopen
> > +  - async-ca
> > +  - dq-ser-mode
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        dramc: dramc@10230000 {
> 
> memory-controller@
> and drop unused label.
> 
> Best regards,
> Krzysztof

Thanks for your review.
Following your suggestion, the v2 patch has been pushed:
https://patchwork.kernel.org/patch/13964209
- Remove redundant "dt-bindings" in subject
- Refine the yaml format
- Change compatible to "mediatek,mt8196-dramc"
- Drop unused label
- Move fmeter-version to platform data

Thanks
Crystal

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Krzysztof Kozlowski]

On 08/02/2025 05:40, Crystal Guo (郭晶) wrote:
> On Thu, 2024-12-12 at 11:58 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On 12/12/2024 09:59, Crystal Guo wrote:
>>> Add devicetree binding for mediatek common-dramc driver.
>>>
>>> The DRAM controller of MediaTek SoC provides an interface to
>>> get the current data rate of DRAM.
>>
>> Bindings are before users.

Do not ignore comments but implement them or keep discussing.

Best regards,
Krzysztof

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 11:46 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 12/12/24 09:59, Crystal Guo ha scritto:
> > Add MediaTek DRAMC driver to provide an interface that can
> > obtain current DDR data rate.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >   drivers/memory/Kconfig                 |   1 +
> >   drivers/memory/Makefile                |   1 +
> >   drivers/memory/mediatek/Kconfig        |  21 ++
> >   drivers/memory/mediatek/Makefile       |   2 +
> >   drivers/memory/mediatek/mtk-dramc.c    | 325
> > +++++++++++++++++++++++++
> >   include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
> >   6 files changed, 391 insertions(+)
> >   create mode 100644 drivers/memory/mediatek/Kconfig
> >   create mode 100644 drivers/memory/mediatek/Makefile
> >   create mode 100644 drivers/memory/mediatek/mtk-dramc.c
> >   create mode 100644 include/linux/soc/mediatek/mtk-dramc.h
> > 
> > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > index c82d8d8a16ea..b1698549ff81 100644
> > --- a/drivers/memory/Kconfig
> > +++ b/drivers/memory/Kconfig
> > @@ -227,5 +227,6 @@ config STM32_FMC2_EBI
> > 
> >   source "drivers/memory/samsung/Kconfig"
> >   source "drivers/memory/tegra/Kconfig"
> > +source "drivers/memory/mediatek/Kconfig"
> > 
> >   endif
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index d2e6ca9abbe0..cf1091449d2e 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)        += stm32-
> > fmc2-ebi.o
> > 
> >   obj-$(CONFIG_SAMSUNG_MC)    += samsung/
> >   obj-$(CONFIG_TEGRA_MC)              += tegra/
> > +obj-$(CONFIG_HAVE_MTK_MC)    += mediatek/
> >   obj-$(CONFIG_TI_EMIF_SRAM)  += ti-emif-sram.o
> >   obj-$(CONFIG_FPGA_DFL_EMIF) += dfl-emif.o
> > 
> > diff --git a/drivers/memory/mediatek/Kconfig
> > b/drivers/memory/mediatek/Kconfig
> > new file mode 100644
> > index 000000000000..00764cdb157e
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Kconfig
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config HAVE_MTK_MC
> > +     bool "MediaTek Memory Controller support"
> > +     help
> > +       This option allows to enable MediaTek memory controller
> > drivers,
> > +       which may include controllers for DRAM or others.
> > +       Select Y here if you need support for MediaTek memory
> > controller.
> > +       If you don't need, select N.
> > +
> > +if HAVE_MTK_MC
> > +
> > +config MTK_DRAMC
> > +     tristate "MediaTek DRAMC driver"
> > +     depends on HAVE_MTK_MC
> > +     help
> > +       This option selects the MediaTek DRAMC driver, which
> > provides
> > +       an interface for reporting the current data rate of DRAM.
> > +       Select Y here if you need support for the MediaTek DRAMC
> > driver.
> > +       If you don't need, select N.
> > +
> > +endif
> > diff --git a/drivers/memory/mediatek/Makefile
> > b/drivers/memory/mediatek/Makefile
> > new file mode 100644
> > index 000000000000..a1395fc55b41
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_MTK_DRAMC)              += mtk-dramc.o
> > diff --git a/drivers/memory/mediatek/mtk-dramc.c
> > b/drivers/memory/mediatek/mtk-dramc.c
> > new file mode 100644
> > index 000000000000..9c2c8e187a4a
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/mtk-dramc.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/printk.h>
> > +#include <linux/io.h>
> > +#include <linux/soc/mediatek/mtk-dramc.h>
> > +
> > +static struct platform_device *dramc_pdev;
> > +static struct platform_driver dramc_drv;
> 
> You don't need these globals.

Okay, remove these globals in v2 patch.

> 
> > +
> > +static int fmeter_init(struct platform_device *pdev,
> > +                    struct fmeter_dev_t *fmeter_dev_ptr, unsigned
> > int fmeter_version)
> > +{
> > +     struct device_node *dramc_node = pdev->dev.of_node;
> > +     int ret;
> > +
> > +     ret = of_property_read_u32(dramc_node,
> > +                                "crystal-freq", &(fmeter_dev_ptr-
> > >crystal_freq));
> > +     ret |= of_property_read_u32(dramc_node,
> > +                                 "shu-of", &(fmeter_dev_ptr-
> > >shu_of));
> 
> ret |= x doesn't make sense.
> 
> ret = ...
> if (ret)
>         return ret;

Okay, remove "ret |=" in v2 patch

> 
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "shu-lv", (unsigned int
> > *)&(fmeter_dev_ptr->shu_lv), 3);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "pll-id", (unsigned int
> > *)&(fmeter_dev_ptr->pll_id), 3);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "sdmpcw", (unsigned int
> > *)(fmeter_dev_ptr->sdmpcw), 6);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "posdiv", (unsigned int
> > *)(fmeter_dev_ptr->posdiv), 6);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "fbksel", (unsigned int
> > *)(fmeter_dev_ptr->fbksel), 6);
> > +     ret |= of_property_read_u32_array(dramc_node,
> > +                                       "dqsopen", (unsigned int
> > *)(fmeter_dev_ptr->dqsopen), 6);
> > +     if (fmeter_version == 1) {
> > +             fmeter_dev_ptr->version = 1;
> > +             ret |= of_property_read_u32_array(dramc_node,
> > +                     "async-ca", (unsigned int *)(fmeter_dev_ptr-
> > >async_ca), 6);
> > +             ret |= of_property_read_u32_array(dramc_node,
> > +                     "dq-ser-mode", (unsigned int
> > *)(fmeter_dev_ptr->dq_ser_mode), 6);
> > +     }
> > +     return ret;
> > +}
> > +
> > +static ssize_t dram_data_rate_show(struct device_driver *driver,
> > char *buf)
> > +{
> > +     return snprintf(buf, PAGE_SIZE, "DRAM data rate = %d\n",
> > +             mtk_dramc_get_data_rate());
> > +}
> > +
> > +static DRIVER_ATTR_RO(dram_data_rate);> +
> > +static int dramc_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *dramc_node = pdev->dev.of_node;
> > +     struct dramc_dev_t *dramc_dev_ptr;
> > +     unsigned int fmeter_version;
> > +     struct resource *res;
> > +     unsigned int i, size;
> > +     int ret;
> > +
> > +     pr_info("%s: module probe.\n", __func__);
> 
> This pr_info makes no sense. Please remove.

Okay, remove pr_info in v2 patch

> 
> > +     dramc_pdev = pdev;
> > +     dramc_dev_ptr = devm_kmalloc(&pdev->dev,
> > +                                  sizeof(struct dramc_dev_t),
> > GFP_KERNEL);
> > +
> > +     if (!dramc_dev_ptr)
> > +             return -ENOMEM;
> > +
> > +     ret = of_property_read_u32(dramc_node,
> > +                                "support-ch-cnt", &dramc_dev_ptr-
> > >support_ch_cnt);
> > +     if (ret) {
> > +             pr_info("%s: get support_ch_cnt fail\n", __func__);
> 
> that's an error, and you want dev_err_probe().

Instead of pr_info, use dev_err_probe in v2 patch.

> 
> > +             return -EINVAL;
> > +     }
> > +
> > +     dramc_dev_ptr->sleep_base = of_iomap(dramc_node,
> > +                                          dramc_dev_ptr-
> > >support_ch_cnt * 4);
> > +     if (IS_ERR(dramc_dev_ptr->sleep_base)) {
> > +             pr_info("%s: unable to map sleep base\n", __func__);
> 
> same, here and everywhere else

Okay, remove all pr_info in v2 patch.

> 
> > +             return -EINVAL;
> > +     }
> > +
> > +     size = sizeof(phys_addr_t) * dramc_dev_ptr->support_ch_cnt;
> > +     dramc_dev_ptr->dramc_chn_base_ao = devm_kmalloc(&pdev->dev,
> > +                                                     size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->dramc_chn_base_ao))
> > +             return -ENOMEM;
> > +     dramc_dev_ptr->dramc_chn_base_nao = devm_kmalloc(&pdev->dev,
> > +                                                      size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->dramc_chn_base_nao))
> > +             return -ENOMEM;
> > +     dramc_dev_ptr->ddrphy_chn_base_ao = devm_kmalloc(&pdev->dev,
> > +                                                      size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->ddrphy_chn_base_ao))
> > +             return -ENOMEM;
> > +     dramc_dev_ptr->ddrphy_chn_base_nao = devm_kmalloc(&pdev->dev,
> > +                                                       size,
> > GFP_KERNEL);
> > +     if (!(dramc_dev_ptr->ddrphy_chn_base_nao))
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < dramc_dev_ptr->support_ch_cnt; i++) {
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > +             dramc_dev_ptr->dramc_chn_base_ao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->dramc_chn_base_ao[i])) {
> > +                     pr_info("%s: unable to map ch%d DRAMC AO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM,
> > +                                         i + dramc_dev_ptr-
> > >support_ch_cnt);
> > +             dramc_dev_ptr->dramc_chn_base_nao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->dramc_chn_base_nao[i])) {
> > +                     pr_info("%s: unable to map ch%d DRAMC NAO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM,
> > +                                         i + dramc_dev_ptr-
> > >support_ch_cnt * 2);
> > +             dramc_dev_ptr->ddrphy_chn_base_ao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_ao[i])) {
> > +                     pr_info("%s: unable to map ch%d DDRPHY AO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM,
> > +                                         i + dramc_dev_ptr-
> > >support_ch_cnt * 3);
> > +             dramc_dev_ptr->ddrphy_chn_base_nao[i] =
> > +                     devm_ioremap_resource(&pdev->dev, res);
> > +             if (IS_ERR(dramc_dev_ptr->ddrphy_chn_base_nao[i])) {
> > +                     pr_info("%s: unable to map ch%d DDRPHY NAO
> > base\n",
> > +                             __func__, i);
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     ret = of_property_read_u32(dramc_node, "fmeter-version",
> > &fmeter_version);
> 
> This is something for platform data, not devicetree.
> 

Okay, already move "fmeter-version" to platform data in v2 patch.

> > +     if (ret) {
> > +             pr_info("%s: get fmeter_version fail\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +     pr_info("%s: fmeter_version(%d)\n", __func__,
> > fmeter_version);
> > +
> > +     if (fmeter_version == 1) {
> > +             dramc_dev_ptr->fmeter_dev_ptr = devm_kmalloc(&pdev-
> > >dev,
> > +                                                          sizeof(s
> > truct fmeter_dev_t),
> > +                                                          GFP_KERN
> > EL);
> > +             if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
> > +                     pr_info("%s: memory  alloc fail\n",
> > __func__);
> > +                     return -ENOMEM;
> > +             }
> > +             ret = fmeter_init(pdev, dramc_dev_ptr-
> > >fmeter_dev_ptr, fmeter_version);
> > +             if (ret) {
> > +                     pr_info("%s: fmeter_init fail\n", __func__);
> > +                     return -EINVAL;
> > +             }
> > +     } else {
> > +             dramc_dev_ptr->fmeter_dev_ptr = NULL;
> > +     }
> > +     ret = driver_create_file(pdev->dev.driver,
> > &driver_attr_dram_data_rate);
> > +     if (ret) {
> > +             pr_info("%s: fail to create dram_data_rate sysfs\n",
> > __func__);
> > +             return ret;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, dramc_dev_ptr);
> > +     pr_info("%s: DRAM data rate = %d\n", __func__,
> > +             mtk_dramc_get_data_rate());
> > +
> > +     return ret;
> > +}
> > +
> > +static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
> > +{
> > +     struct fmeter_dev_t *fmeter_dev_ptr =
> > +             (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> > +     unsigned int shu_lv_val;
> > +     unsigned int pll_id_val;
> > +     unsigned int sdmpcw_val;
> > +     unsigned int posdiv_val;
> > +     unsigned int ckdiv4_val;
> > +     unsigned int offset;
> > +     unsigned int vco_freq;
> > +     unsigned int fbksel;
> > +     unsigned int dqsopen;
> > +     unsigned int async_ca;
> > +     unsigned int dq_ser_mode;
> > +
> > +     shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->shu_lv.offset) &
> > +             fmeter_dev_ptr->shu_lv.mask) >>
> > +             fmeter_dev_ptr->shu_lv.shift;
> > +
> > +     pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->pll_id.offset) &
> > +             fmeter_dev_ptr->pll_id.mask) >>
> > +             fmeter_dev_ptr->pll_id.shift;
> > +
> > +     offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->posdiv[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->fbksel[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->async_ca[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
> > +     ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3:
> > DIV16
> > +
> > +     posdiv_val &= ~(0x4);
> > +
> > +     vco_freq = ((fmeter_dev_ptr->crystal_freq) *
> > +             (sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
> > +             << fbksel;
> 
> That vco_freq calculation is seriously unreadable :\
> 
In v2 patch, simplify this calculation process to:
	...
	perdiv_freq = REF_FREQUENCY * (sdmpcw >> PREDIV);
	posdiv_freq = (perdiv_freq >> posdiv) >> 1;
	vco_freq = posdiv_freq << fbksel;
	final_rate = vco_freq >> ckdiv4;

	if (sopen == 1 && async_ca == 1)
		final_rate >>= 1;

> > +
> > +     if ((dqsopen == 1) && (async_ca == 1))
> > +             vco_freq >>= 1;
> > +
> > +     return vco_freq;
> > +}
> > +
> > +/*
> > + * mtk_dramc_get_data_rate - calculate DRAM data rate
> > + *
> > + * Returns DRAM data rate (MB/s)
> > + */
> > +unsigned int mtk_dramc_get_data_rate(void)
> > +{
> > +     struct dramc_dev_t *dramc_dev_ptr;
> > +     struct fmeter_dev_t *fmeter_dev_ptr;
> > +
> > +     if (!dramc_pdev)
> > +             return 0;
> > +
> > +     dramc_dev_ptr =
> > +             (struct dramc_dev_t
> > *)platform_get_drvdata(dramc_pdev);
> > +
> > +     fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr-
> > >fmeter_dev_ptr;
> > +     if (!fmeter_dev_ptr)
> > +             return 0;
> > +
> > +     if (fmeter_dev_ptr->version == 1)
> > +             return fmeter_v1(dramc_dev_ptr);
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(mtk_dramc_get_data_rate);
> 
> Not sure about mtk_dramc_get_data_rate().
> 
> Please show an example of how you're actually using it in the
> external driver,
> as there are many ways to expose this value.
> 

Remove the export of mtk_dramc_get_data_rate().
The current dram data rate can be got by sysfs node.

> > +
> > +static int dramc_remove(struct platform_device *pdev)
> > +{
> > +     dramc_pdev = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id dramc_of_ids[] = {
> > +     {.compatible = "mediatek,common-dramc",},
> 
> { .compatible = "mediatek,something" },
> { /* sentinel */ }
> 

Change compatible to "mediatek,mt8196-dramc"

> > +     {}
> > +};
> > +
> > +static struct platform_driver dramc_drv = {
> > +     .probe = dramc_probe,
> > +     .remove = dramc_remove,
> > +     .driver = {
> > +             .name = "dramc_drv",
> > +             .owner = THIS_MODULE,
> > +             .of_match_table = dramc_of_ids,
> > +     },
> > +};
> 
> module_platform_driver(...)
> 
> > +
> > +static int __init dramc_drv_init(void)
> 
> ...so that function is not needed.
> 

Okay, apply the change in v2 patch.

> > +{
> > +     int ret;
> > +
> > +     ret = platform_driver_register(&dramc_drv);
> > +     if (ret) {
> > +             pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +module_init(dramc_drv_init);
> > +
> > +MODULE_AUTHOR("Mediatek Corporation");
> 
> MODULE_AUTHOR("Author Name <your.email@mediatek.com>");
> 
> > +MODULE_DESCRIPTION("MediaTek DRAMC Driver");
> 
> MODULE_DESCRIPTION("MediaTek DRAM Controller driver");
> 
> 
> 
> ...and I have a hunch that this series will need many versions before
> the code
> quality bar reaches a nice level...
> 
> Cheers,
> Angelo
> 
> > +MODULE_LICENSE("GPL");

Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 12:02 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 12/12/2024 09:59, Crystal Guo wrote:
> > Add MediaTek DRAMC driver to provide an interface that can
> > obtain current DDR data rate.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >  drivers/memory/Kconfig                 |   1 +
> >  drivers/memory/Makefile                |   1 +
> >  drivers/memory/mediatek/Kconfig        |  21 ++
> >  drivers/memory/mediatek/Makefile       |   2 +
> >  drivers/memory/mediatek/mtk-dramc.c    | 325
> > +++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-dramc.h |  41 ++++
> >  6 files changed, 391 insertions(+)
> >  create mode 100644 drivers/memory/mediatek/Kconfig
> >  create mode 100644 drivers/memory/mediatek/Makefile
> >  create mode 100644 drivers/memory/mediatek/mtk-dramc.c
> >  create mode 100644 include/linux/soc/mediatek/mtk-dramc.h
> > 
> > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > index c82d8d8a16ea..b1698549ff81 100644
> > --- a/drivers/memory/Kconfig
> > +++ b/drivers/memory/Kconfig
> > @@ -227,5 +227,6 @@ config STM32_FMC2_EBI
> > 
> >  source "drivers/memory/samsung/Kconfig"
> >  source "drivers/memory/tegra/Kconfig"
> > +source "drivers/memory/mediatek/Kconfig"
> > 
> >  endif
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index d2e6ca9abbe0..cf1091449d2e 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_STM32_FMC2_EBI)        += stm32-
> > fmc2-ebi.o
> > 
> >  obj-$(CONFIG_SAMSUNG_MC)     += samsung/
> >  obj-$(CONFIG_TEGRA_MC)               += tegra/
> > +obj-$(CONFIG_HAVE_MTK_MC)    += mediatek/
> 
> What is HAVE? Why this is not just MTK_MC or ARCH_MEDIATEK?

Rename the dramc config to "CONFIG_MEDIATEK_MC" in v2 patch.

> 
> >  obj-$(CONFIG_TI_EMIF_SRAM)   += ti-emif-sram.o
> >  obj-$(CONFIG_FPGA_DFL_EMIF)  += dfl-emif.o
> > 
> > diff --git a/drivers/memory/mediatek/Kconfig
> > b/drivers/memory/mediatek/Kconfig
> > new file mode 100644
> > index 000000000000..00764cdb157e
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Kconfig
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config HAVE_MTK_MC
> > +     bool "MediaTek Memory Controller support"
> > +     help
> > +       This option allows to enable MediaTek memory controller
> > drivers,
> > +       which may include controllers for DRAM or others.
> > +       Select Y here if you need support for MediaTek memory
> > controller.
> > +       If you don't need, select N.
> > +
> > +if HAVE_MTK_MC
> > +
> > +config MTK_DRAMC
> > +     tristate "MediaTek DRAMC driver"
> > +     depends on HAVE_MTK_MC
> 
> Feels useless, isn't it part of if?

Change to "default y".

> 
> > +     help
> > +       This option selects the MediaTek DRAMC driver, which
> > provides
> > +       an interface for reporting the current data rate of DRAM.
> > +       Select Y here if you need support for the MediaTek DRAMC
> > driver.
> > +       If you don't need, select N.
> > +
> > +endif
> > diff --git a/drivers/memory/mediatek/Makefile
> > b/drivers/memory/mediatek/Makefile
> > new file mode 100644
> > index 000000000000..a1395fc55b41
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_MTK_DRAMC)              += mtk-dramc.o
> > diff --git a/drivers/memory/mediatek/mtk-dramc.c
> > b/drivers/memory/mediatek/mtk-dramc.c
> > new file mode 100644
> > index 000000000000..9c2c8e187a4a
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/mtk-dramc.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/printk.h>
> > +#include <linux/io.h>
> > +#include <linux/soc/mediatek/mtk-dramc.h>
> > +
> > +static struct platform_device *dramc_pdev;
> > +static struct platform_driver dramc_drv;
> 
> Drop both. Don't write singleton code.

Okay, removed these code in v2.
> 
> ...
> 
> > +             if (!(dramc_dev_ptr->fmeter_dev_ptr)) {
> > +                     pr_info("%s: memory  alloc fail\n",
> > __func__);
> > +                     return -ENOMEM;
> > +             }
> > +             ret = fmeter_init(pdev, dramc_dev_ptr-
> > >fmeter_dev_ptr, fmeter_version);
> > +             if (ret) {
> > +                     pr_info("%s: fmeter_init fail\n", __func__);
> > +                     return -EINVAL;
> > +             }
> > +     } else {
> > +             dramc_dev_ptr->fmeter_dev_ptr = NULL;
> > +     }
> > +     ret = driver_create_file(pdev->dev.driver,
> > &driver_attr_dram_data_rate);
> > +     if (ret) {
> > +             pr_info("%s: fail to create dram_data_rate sysfs\n",
> > __func__);
> > +             return ret;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, dramc_dev_ptr);
> > +     pr_info("%s: DRAM data rate = %d\n", __func__,
> > +             mtk_dramc_get_data_rate());
> 
> 
> This is terible coding style. Driver is in very poor shape.
> 
> Please perform extensive internal review in Mediatek to avoid sending
> very poor quality contributions. This uses community resources in
> unfair
> way. Few more comments, but all this code needs extensive rework and
> I
> am not going to explain you basics of Linux Coding style, if you do
> not
> want to take existing code as learning examples. Work with your
> collegagues on that.
> 
> 
> > +
> > +     return ret;
> > +}
> > +
> > +static unsigned int fmeter_v1(struct dramc_dev_t *dramc_dev_ptr)
> > +{
> > +     struct fmeter_dev_t *fmeter_dev_ptr =
> > +             (struct fmeter_dev_t *)dramc_dev_ptr->fmeter_dev_ptr;
> > +     unsigned int shu_lv_val;
> > +     unsigned int pll_id_val;
> > +     unsigned int sdmpcw_val;
> > +     unsigned int posdiv_val;
> > +     unsigned int ckdiv4_val;
> > +     unsigned int offset;
> > +     unsigned int vco_freq;
> > +     unsigned int fbksel;
> > +     unsigned int dqsopen;
> > +     unsigned int async_ca;
> > +     unsigned int dq_ser_mode;
> > +
> > +     shu_lv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->shu_lv.offset) &
> > +             fmeter_dev_ptr->shu_lv.mask) >>
> > +             fmeter_dev_ptr->shu_lv.shift;
> > +
> > +     pll_id_val = (readl(dramc_dev_ptr->ddrphy_chn_base_ao[0] +
> > +             fmeter_dev_ptr->pll_id.offset) &
> > +             fmeter_dev_ptr->pll_id.mask) >>
> > +             fmeter_dev_ptr->pll_id.shift;
> > +
> > +     offset = fmeter_dev_ptr->sdmpcw[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     sdmpcw_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->sdmpcw[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->posdiv[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     posdiv_val = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->posdiv[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->posdiv[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->fbksel[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     fbksel = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->fbksel[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->fbksel[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dqsopen[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dqsopen = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dqsopen[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->async_ca[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     async_ca = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->async_ca[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->async_ca[pll_id_val].shift;
> > +
> > +     offset = fmeter_dev_ptr->dq_ser_mode[pll_id_val].offset +
> > +             fmeter_dev_ptr->shu_of * shu_lv_val;
> > +     dq_ser_mode = (readl(dramc_dev_ptr->ddrphy_chn_base_nao[0] +
> > offset) &
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].mask) >>
> > +             fmeter_dev_ptr->dq_ser_mode[pll_id_val].shift;
> > +     ckdiv4_val = (dq_ser_mode == 1); // 1: DIV4, 2: DIV8, 3:
> > DIV16
> > +
> > +     posdiv_val &= ~(0x4);
> > +
> > +     vco_freq = ((fmeter_dev_ptr->crystal_freq) *
> > +             (sdmpcw_val >> 7)) >> posdiv_val >> 1 >> ckdiv4_val
> > +             << fbksel;
> > +
> > +     if ((dqsopen == 1) && (async_ca == 1))
> > +             vco_freq >>= 1;
> > +
> > +     return vco_freq;
> > +}
> > +
> > +/*
> > + * mtk_dramc_get_data_rate - calculate DRAM data rate
> > + *
> > + * Returns DRAM data rate (MB/s)
> > + */
> > +unsigned int mtk_dramc_get_data_rate(void)
> > +{
> > +     struct dramc_dev_t *dramc_dev_ptr;
> > +     struct fmeter_dev_t *fmeter_dev_ptr;
> > +
> > +     if (!dramc_pdev)
> > +             return 0;
> > +
> > +     dramc_dev_ptr =
> > +             (struct dramc_dev_t
> > *)platform_get_drvdata(dramc_pdev);
> > +
> > +     fmeter_dev_ptr = (struct fmeter_dev_t *)dramc_dev_ptr-
> > >fmeter_dev_ptr;
> > +     if (!fmeter_dev_ptr)
> > +             return 0;
> > +
> > +     if (fmeter_dev_ptr->version == 1)
> > +             return fmeter_v1(dramc_dev_ptr);
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(mtk_dramc_get_data_rate);
> 
> NAK, not used.

Okay, change the declaration of mtk_dramc_get_data_rate() to static.

> 
> > +
> > +static int dramc_remove(struct platform_device *pdev)
> > +{
> > +     dramc_pdev = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id dramc_of_ids[] = {
> > +     {.compatible = "mediatek,common-dramc",},
> > +     {}
> > +};
> > +
> > +static struct platform_driver dramc_drv = {
> > +     .probe = dramc_probe,
> > +     .remove = dramc_remove,
> > +     .driver = {
> > +             .name = "dramc_drv",
> > +             .owner = THIS_MODULE,
> 
> NAK. You sent us 10 year old code, which means you replicated all the
> bugs, issues and old-coding styles existing 10 years ago.
> 
> Start *FROM SCRATCH* from the newest driver, so you won't send us
> same
> junk we already fixed during last 10 years.
> 
> 
> > +             .of_match_table = dramc_of_ids,
> > +     },
> > +};
> > +
> > +static int __init dramc_drv_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = platform_driver_register(&dramc_drv);
> > +     if (ret) {
> > +             pr_info("%s: init fail, ret 0x%x\n", __func__, ret);
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +module_init(dramc_drv_init);
> 
> No, just module_platform_driver

Okay, remove module_init in v2.

> 
> > +
> > +MODULE_AUTHOR("Mediatek Corporation");
> > +MODULE_DESCRIPTION("MediaTek DRAMC Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/soc/mediatek/mtk-dramc.h
> > b/include/linux/soc/mediatek/mtk-dramc.h
> > new file mode 100644
> > index 000000000000..95e7dbfe7d0e
> > --- /dev/null
> > +++ b/include/linux/soc/mediatek/mtk-dramc.h
> 
> Move everything into driver.

Okay.

> 
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_DRAMC_H__
> > +#define __MTK_DRAMC_H__
> > +
> > +struct reg_ctrl_t {
> > +     unsigned int offset;
> > +     unsigned int mask;
> > +     unsigned int shift;
> > +};
> > +
> > +struct fmeter_dev_t {
> > +     unsigned int version;
> > +     unsigned int crystal_freq;
> > +     unsigned int shu_of;
> > +     struct reg_ctrl_t shu_lv;
> > +     struct reg_ctrl_t pll_id;
> > +     struct reg_ctrl_t sdmpcw[2];
> > +     struct reg_ctrl_t posdiv[2];
> > +     struct reg_ctrl_t fbksel[2];
> > +     struct reg_ctrl_t dqsopen[2];
> > +     struct reg_ctrl_t async_ca[2];
> > +     struct reg_ctrl_t dq_ser_mode[2];
> > +};
> > +
> > +struct dramc_dev_t {
> > +     unsigned int support_ch_cnt;
> > +     void __iomem **dramc_chn_base_ao;
> > +     void __iomem **dramc_chn_base_nao;
> > +     void __iomem **ddrphy_chn_base_ao;
> > +     void __iomem **ddrphy_chn_base_nao;
> > +     void __iomem *sleep_base;
> > +     void *fmeter_dev_ptr;
> > +};
> > +
> > +unsigned int mtk_dramc_get_data_rate(void);
> 
> 
> Drop, unused.

Okay, remove mtk-dramc.h.
> 
> > +
> > +#endif /* __MTK_DRAMC_H__ */
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 11:27 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 12/12/24 09:59, Crystal Guo ha scritto:
> > Add devicetree binding for mediatek common-dramc driver.
> > 
> > The DRAM controller of MediaTek SoC provides an interface to
> > get the current data rate of DRAM.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >   .../mediatek,common-dramc.yaml                | 129
> > ++++++++++++++++++
> >   1 file changed, 129 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > new file mode 100644
> > index 000000000000..c9e608c7f183
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +# Copyright (c) 2024 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml*__;Iw!!CTRNKA9wMg0ARbw!hkObFeP7J-uvNSyDsepqqdH_jZQcf9NQ1knBRGY1ODpH6FoZyBzL1x5rEIWPppp1wNmLdo41PQTZM4ulMP1Qg7wr9PwFEdeN$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!hkObFeP7J-uvNSyDsepqqdH_jZQcf9NQ1knBRGY1ODpH6FoZyBzL1x5rEIWPppp1wNmLdo41PQTZM4ulMP1Qg7wr9FN7m95w$
> > +
> > +title: MediaTek Common DRAMC (DRAM Controller)
> 
> MediaTek DRAM Controller (DRAMC)

Okay, change title to "MediaTek DRAM Controller (DRAMC)"

> 
> > +
> > +maintainers:
> > +  - Crystal Guo <crystal.guo@mediatek.com>
> > +
> > +description: |
> > +  The DRAM controller of MediaTek SoC provides an interface to
> > +  get the current data rate of DRAM.
> 
> No, the DRAM Controller does much more than just that.
> 

Change the description to "A MediaTek DRAM controller interface to
provide the current data rate of DRAM.", is it okay?.

> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,common-dramc
> 
> Absolutely no! Compatibles are per-soc.
> 
> mediatek,mt8186-dramc
> mediatek,mt8188-dramc
> mediatek,mt8195-dramc
> 
> etc
> 

Change the compatible to "mediatek,mt8196-dramc"

> > +
> > +  reg:
> > +    minItems: 9
> > +    items:
> > +      - description: DRAMC_AO_CHA_BASE
> > +      - description: DRAMC_AO_CHB_BASE
> > +      - description: DRAMC_AO_CHC_BASE
> > +      - description: DRAMC_AO_CHD_BASE
> 
> All those channels are sequential in AO->NAO, in the sense that
> every channel is:
> 
> CH0     AO: 0x10230000   len: 0x4000
> CH0    NAO: 0x10234000   len: 0x2000
> CH0 PHY_AO: 0x10236000   len: 0x2000
> CH0 PHY_AO: 0x10238000   len: 0x2000
> 
> So the reg can be simplified as
> 
> minItems: 4
> items:
>    - description: DRAM Controller Channel 0
>    - description: DRAM Controller Channel 1
>    - description: DRAM Controller Channel 2
>    - description: DRAM Controller Channel 3
> 
> 
> > +      - description: DRAMC_NAO_CHA_BASE
> > +      - description: DRAMC_NAO_CHB_BASE
> > +      - description: DRAMC_NAO_CHC_BASE
> > +      - description: DRAMC_NAO_CHD_BASE
> > +      - description: DDRPHY_AO_CHA_BASE
> > +      - description: DDRPHY_AO_CHB_BASE
> > +      - description: DDRPHY_AO_CHC_BASE
> > +      - description: DDRPHY_AO_CHD_BASE
> > +      - description: DDRPHY_NAO_CHA_BASE
> > +      - description: DDRPHY_NAO_CHB_BASE
> > +      - description: DDRPHY_NAO_CHC_BASE
> > +      - description: DDRPHY_NAO_CHD_BASE
> > +      - description: SLEEP_BASE
> 
> You're not using the SLEEP_BASE iospace, and that's not even really
> specific
> to the DRAM Controller. Drop it.

Actually the driver only use DDRPHY_AO_CHA_BASE and
DDRPHY_NAO_CHA_BASE,
Thus simplify the reg properity to:
  reg:
    items:
      - description: anaphy registers
      - description: ddrphy registers
> 
> > +
> > +  support-ch-cnt:
> > +    maxItems: 1
> 
> Don't tell me that the DRAM Controller in MediaTek SoCs cannot see
> how many
> channels are actually occupied by a DRAM bank, because I will be
> really skeptical.
> 
> You can autodetect that in the driver, you don't need a DT property
> for that.

Okay, remove the property "support-ch-cnt".
> 
> > +
> > +  fmeter-version:
> > +    maxItems: 1
> > +    description:
> > +      Fmeter version for calculating dram data rate
> 
> The Fmeter version is SoC-specific, you need platform data, not DT
> property.
> 

Okay, move the "fmeter-version" to platform data.

> > +
> > +  crystal-freq:
> > +    maxItems: 1
> > +    description:
> > +      Reference clock rate in MHz
> 
> Is this crystal an external component, or is it integrated into the
> SoC?

This is the reference frequency used to calculate the DDR frequency.
In v2, the "crystal-freq" property has been removed and defined
directly in the driver.

> 
> > +
> > +  shu-of:
> > +    maxItems: 1
> 
> There's no description, what is shu-of?
> 
> > +
> > +  pll-id: true
> > +  shu-lv: true
> > +  sdmpcw: true
> > +  posdiv: true
> > +  fbksel: true
> > +  dqsopen: true
> > +  async-ca: true
> > +  dq-ser-mode: true
> 
> Same for these ones, please describe them - but then remember: if
> those parameters
> are board-specific, they can stay here, otherwise those go in
> platform data.
> 
> Besides, I doubt that those are board specific.
> 
> Regards,
> Angelo
> 

These parameters are SoC-specific.
In v2, the offsets of the registers storing these parameters are
defined directly in the driver.
THus these parameters can be removed from DT property.

Thanks
Crystal

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Thu, 2024-12-12 at 11:58 +0100, Krzysztof Kozlowski wrote:
> > 

> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 12/12/2024 09:59, Crystal Guo wrote:
> > Add devicetree binding for mediatek common-dramc driver.
> > 
> > The DRAM controller of MediaTek SoC provides an interface to
> > get the current data rate of DRAM.
> 
> 
> 
> 
> 
> A nit, subject: drop second/last, redundant "dt-bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> 
https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst*L18__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5DM7MWDx$
> 

Okay, remove the redundant "dt-bindings", change the subject to "dt-
bindings: memory-controllers: Add MediaTek DRAM controller interface"


> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >  .../mediatek,common-dramc.yaml                | 129
> > ++++++++++++++++++
> >  1 file changed, 129 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > new file mode 100644
> > index 000000000000..c9e608c7f183
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,common-dramc.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +# Copyright (c) 2024 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5GD5-Mgk$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5AGE5Eci$
> > +
> > +title: MediaTek Common DRAMC (DRAM Controller)
> 
> Common? Is this a real thing? Please describe the hardware.
> 

Sorry, my original description was not accurate. It has been changed
to:

Title: MediaTek DRAM Controller (DRAMC)

> > +
> > +maintainers:
> > +  - Crystal Guo <crystal.guo@mediatek.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 

Okay, remove it in v2.

> > +  The DRAM controller of MediaTek SoC provides an interface to
> > +  get the current data rate of DRAM.
> 
> So not common here?

Sorry, my original title description was not accurate.

> 
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,common-dramc
> 
> This has to be SoC.
> 

Change to "mediatek,mt8196-dramc"

> > +
> > +  reg:
> > +    minItems: 9
> 
> Why this is flexible?
> 
The original implementation was incorrect and has been corrected in v2
to:

  reg:
    items:
      - description: anaphy registers
      - description: ddrphy registers

> > +    items:
> > +      - description: DRAMC_AO_CHA_BASE
> > +      - description: DRAMC_AO_CHB_BASE
> > +      - description: DRAMC_AO_CHC_BASE
> > +      - description: DRAMC_AO_CHD_BASE
> > +      - description: DRAMC_NAO_CHA_BASE
> > +      - description: DRAMC_NAO_CHB_BASE
> > +      - description: DRAMC_NAO_CHC_BASE
> > +      - description: DRAMC_NAO_CHD_BASE
> > +      - description: DDRPHY_AO_CHA_BASE
> > +      - description: DDRPHY_AO_CHB_BASE
> > +      - description: DDRPHY_AO_CHC_BASE
> > +      - description: DDRPHY_AO_CHD_BASE
> > +      - description: DDRPHY_NAO_CHA_BASE
> > +      - description: DDRPHY_NAO_CHB_BASE
> > +      - description: DDRPHY_NAO_CHC_BASE
> > +      - description: DDRPHY_NAO_CHD_BASE
> > +      - description: SLEEP_BASE
> 
> Don't use some defines. Look at other bindings how they describe
> items.
> 

This part has been updated in v2 to:

  reg:
    items:
      - description: anaphy registers
      - description: ddrphy registers

> > +
> > +  support-ch-cnt:
> 
> Nope

Okay, remove this property.

> 
> > +    maxItems: 1
> > +
> > +  fmeter-version:
> > +    maxItems: 1
> > +    description:
> > +      Fmeter version for calculating dram data rate
> > +
> > +  crystal-freq:
> > +    maxItems: 1
> > +    description:
> > +      Reference clock rate in MHz
> > +
> > +  shu-of:
> > +    maxItems: 1
> > +
> > +  pll-id: true
> > +  shu-lv: true
> > +  sdmpcw: true
> > +  posdiv: true
> > +  fbksel: true
> > +  dqsopen: true
> > +  async-ca: true
> > +  dq-ser-mode: true
> 
> 
> This binding is terrible. Was not tested and does not follow any
> guidelines. Please read example schema and writing bindings document.
> You can also read slides from my talks...
> 
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - support-ch-cnt
> > +  - fmeter-version
> > +  - crystal-freq
> > +  - pll-id
> > +  - shu-lv
> > +  - shu-of
> > +  - sdmpcw
> > +  - posdiv
> > +  - fbksel
> > +  - dqsopen
> > +  - async-ca
> > +  - dq-ser-mode
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        dramc: dramc@10230000 {
> 
> memory-controller@
> and drop unused label.

Okay, thanks.
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Sun, 2025-02-09 at 11:52 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 08/02/2025 05:40, Crystal Guo (郭晶) wrote:
> > On Thu, 2024-12-12 at 11:58 +0100, Krzysztof Kozlowski wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On 12/12/2024 09:59, Crystal Guo wrote:
> > > > Add devicetree binding for mediatek common-dramc driver.
> > > > 
> > > > The DRAM controller of MediaTek SoC provides an interface to
> > > > get the current data rate of DRAM.
> > > 
> > > Bindings are before users.
> 
> Do not ignore comments but implement them or keep discussing.
> 
> Best regards,
> Krzysztof

Got it, I have replied to the previous comments. Thanks for the
reminder.

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Krzysztof Kozlowski]

On 11/02/2025 13:56, Crystal Guo (郭晶) wrote:
>>> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
>>> ---
>>>  .../mediatek,common-dramc.yaml                | 129
>>> ++++++++++++++++++
>>>  1 file changed, 129 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/memory-
>>> controllers/mediatek,common-dramc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-
>>> controllers/mediatek,common-dramc.yaml
>>> b/Documentation/devicetree/bindings/memory-
>>> controllers/mediatek,common-dramc.yaml
>>> new file mode 100644
>>> index 000000000000..c9e608c7f183
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/memory-
>>> controllers/mediatek,common-dramc.yaml
>>> @@ -0,0 +1,129 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +# Copyright (c) 2024 MediaTek Inc.
>>> +%YAML 1.2
>>> +---
>>> +$id: 
>>> https://urldefense.com/v3/__http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5GD5-Mgk$
>>> +$schema: 
>>> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5AGE5Eci$
>>> +
>>> +title: MediaTek Common DRAMC (DRAM Controller)
>>
>> Common? Is this a real thing? Please describe the hardware.
>>
> 
> Sorry, my original description was not accurate. It has been changed
> to:
> 
> Title: MediaTek DRAM Controller (DRAMC)
> 
>>> +
>>> +maintainers:
>>> +  - Crystal Guo <crystal.guo@mediatek.com>
>>> +
>>> +description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
> 
> Okay, remove it in v2.
> 
>>> +  The DRAM controller of MediaTek SoC provides an interface to
>>> +  get the current data rate of DRAM.
>>
>> So not common here?
> 
> Sorry, my original title description was not accurate.
> 
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: mediatek,common-dramc
>>
>> This has to be SoC.
>>
> 
> Change to "mediatek,mt8196-dramc"
> 
>>> +
>>> +  reg:
>>> +    minItems: 9
>>
>> Why this is flexible?
>>
> The original implementation was incorrect and has been corrected in v2


You replied to two months old review. I don't have the context and I do
not have these emails in my inbox, therefore if you have any questions I
cannot answer.

Please implement entire feedback or discuss within reasonable amount of
time. If something is unresolved, please mention it in the changelog of
future versions.

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Krzysztof Kozlowski]

On 11/02/2025 13:59, Crystal Guo (郭晶) wrote:
> On Sun, 2025-02-09 at 11:52 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On 08/02/2025 05:40, Crystal Guo (郭晶) wrote:
>>> On Thu, 2024-12-12 at 11:58 +0100, Krzysztof Kozlowski wrote:
>>>> External email : Please do not click links or open attachments
>>>> until
>>>> you have verified the sender or the content.
>>>>
>>>>
>>>> On 12/12/2024 09:59, Crystal Guo wrote:
>>>>> Add devicetree binding for mediatek common-dramc driver.
>>>>>
>>>>> The DRAM controller of MediaTek SoC provides an interface to
>>>>> get the current data rate of DRAM.
>>>>
>>>> Bindings are before users.
>>
>> Do not ignore comments but implement them or keep discussing.
>>
>> Best regards,
>> Krzysztof
> 
> Got it, I have replied to the previous comments. Thanks for the
> reminder.


Ah, so these replies were because of this. Thanks, that makes sense,
although still two months after initial review, so I am out of the context.

I assume above also means you implement the feedback :)

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Tue, 2025-02-11 at 16:55 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 11/02/2025 13:56, Crystal Guo (郭晶) wrote:
> > > > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > > > ---
> > > >  .../mediatek,common-dramc.yaml                | 129
> > > > ++++++++++++++++++
> > > >  1 file changed, 129 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/memory-
> > > > controllers/mediatek,common-dramc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/memory-
> > > > controllers/mediatek,common-dramc.yaml
> > > > b/Documentation/devicetree/bindings/memory-
> > > > controllers/mediatek,common-dramc.yaml
> > > > new file mode 100644
> > > > index 000000000000..c9e608c7f183
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > controllers/mediatek,common-dramc.yaml
> > > > @@ -0,0 +1,129 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +# Copyright (c) 2024 MediaTek Inc.
> > > > +%YAML 1.2
> > > > +---
> > > > +$id:
> > > > 
https://urldefense.com/v3/__http://devicetree.org/schemas/memory-controllers/mediatek,common-dramc.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5GD5-Mgk$
> > > > +$schema:
> > > > 
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!mztYfN3n6_IAx78S44PFOetQS51-h6obm2HHrjEVRI-HJYyzJ2VWbbik2rn3pybssUBOT4gp5AGE5Eci$
> > > > +
> > > > +title: MediaTek Common DRAMC (DRAM Controller)
> > > 
> > > Common? Is this a real thing? Please describe the hardware.
> > > 
> > 
> > Sorry, my original description was not accurate. It has been
> > changed
> > to:
> > 
> > Title: MediaTek DRAM Controller (DRAMC)
> > 
> > > > +
> > > > +maintainers:
> > > > +  - Crystal Guo <crystal.guo@mediatek.com>
> > > > +
> > > > +description: |
> > > 
> > > Do not need '|' unless you need to preserve formatting.
> > > 
> > 
> > Okay, remove it in v2.
> > 
> > > > +  The DRAM controller of MediaTek SoC provides an interface to
> > > > +  get the current data rate of DRAM.
> > > 
> > > So not common here?
> > 
> > Sorry, my original title description was not accurate.
> > 
> > > 
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: mediatek,common-dramc
> > > 
> > > This has to be SoC.
> > > 
> > 
> > Change to "mediatek,mt8196-dramc"
> > 
> > > > +
> > > > +  reg:
> > > > +    minItems: 9
> > > 
> > > Why this is flexible?
> > > 
> > 
> > The original implementation was incorrect and has been corrected in
> > v2
> 
> 
> You replied to two months old review. I don't have the context and I
> do
> not have these emails in my inbox, therefore if you have any
> questions I
> cannot answer.
> 
> Please implement entire feedback or discuss within reasonable amount
> of
> time. If something is unresolved, please mention it in the changelog
> of
> future versions.
> 
> Best regards,
> Krzysztof

Okay, got it, thanks.

Best regards,
Crystal

Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings

[Crystal Guo (郭晶)]

On Tue, 2025-02-11 at 16:57 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 11/02/2025 13:59, Crystal Guo (郭晶) wrote:
> > On Sun, 2025-02-09 at 11:52 +0100, Krzysztof Kozlowski wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On 08/02/2025 05:40, Crystal Guo (郭晶) wrote:
> > > > On Thu, 2024-12-12 at 11:58 +0100, Krzysztof Kozlowski wrote:
> > > > > External email : Please do not click links or open
> > > > > attachments
> > > > > until
> > > > > you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > On 12/12/2024 09:59, Crystal Guo wrote:
> > > > > > Add devicetree binding for mediatek common-dramc driver.
> > > > > > 
> > > > > > The DRAM controller of MediaTek SoC provides an interface
> > > > > > to
> > > > > > get the current data rate of DRAM.
> > > > > 
> > > > > Bindings are before users.
> > > 
> > > Do not ignore comments but implement them or keep discussing.
> > > 
> > > Best regards,
> > > Krzysztof
> > 
> > Got it, I have replied to the previous comments. Thanks for the
> > reminder.
> 
> 
> Ah, so these replies were because of this. Thanks, that makes sense,
> although still two months after initial review, so I am out of the
> context.
> 
> I assume above also means you implement the feedback :)
> 
> Best regards,
> Krzysztof

Yes, I have pushed the v2 patch based on these comments:
https://patchwork.kernel.org/project/linux-mediatek/list/?series=931410

Changes in v2:
- Remove pr_info and pr_err, use dev_err or dev_err_probe to print
  error message;
- Replace module_init by module_platform_driver;
- Remove unnecessary global variables;
- Change fmeter-verison to platform data;
- Remove mtk-dramc.h;
- Refine compatible to "mediatek,mt8196-dramc";
- Refine CONFIG name to MEDIATEK_MC;
- Fix yaml build errors, remove unnecessary properties on yaml file.

Please help review this new version, thanks.

Best regards
Crystal