* [PATCH 0/2] Add MediaTek DRAMC driver support @ 2024-12-12 8:59 Crystal Guo 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo 2024-12-12 8:59 ` [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings Crystal Guo 0 siblings, 2 replies; 25+ messages in thread From: Crystal Guo @ 2024-12-12 8:59 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno, Crystal Guo Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 8:59 [PATCH 0/2] Add MediaTek DRAMC driver support Crystal Guo @ 2024-12-12 8:59 ` Crystal Guo 2024-12-12 10:46 ` AngeloGioacchino Del Regno ` (3 more replies) 2024-12-12 8:59 ` [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings Crystal Guo 1 sibling, 4 replies; 25+ messages in thread From: Crystal Guo @ 2024-12-12 8:59 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno, Crystal Guo Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo @ 2024-12-12 10:46 ` AngeloGioacchino Del Regno 2025-02-08 4:14 ` Crystal Guo (郭晶) 2025-02-11 12:13 ` Crystal Guo (郭晶) 2024-12-12 11:02 ` Krzysztof Kozlowski ` (2 subsequent siblings) 3 siblings, 2 replies; 25+ messages in thread From: AngeloGioacchino Del Regno @ 2024-12-12 10:46 UTC (permalink / raw) To: Crystal Guo, Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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"); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 10:46 ` AngeloGioacchino Del Regno @ 2025-02-08 4:14 ` Crystal Guo (郭晶) 2025-02-11 12:13 ` Crystal Guo (郭晶) 1 sibling, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-08 4:14 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, AngeloGioacchino Del Regno, krzk@kernel.org, conor+dt@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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"); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 10:46 ` AngeloGioacchino Del Regno 2025-02-08 4:14 ` Crystal Guo (郭晶) @ 2025-02-11 12:13 ` Crystal Guo (郭晶) 1 sibling, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-11 12:13 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, AngeloGioacchino Del Regno, krzk@kernel.org, conor+dt@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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"); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo 2024-12-12 10:46 ` AngeloGioacchino Del Regno @ 2024-12-12 11:02 ` Krzysztof Kozlowski 2025-02-08 4:21 ` Crystal Guo (郭晶) 2025-02-11 12:21 ` Crystal Guo (郭晶) 2024-12-21 1:42 ` kernel test robot 2024-12-21 11:03 ` kernel test robot 3 siblings, 2 replies; 25+ messages in thread From: Krzysztof Kozlowski @ 2024-12-12 11:02 UTC (permalink / raw) To: Crystal Guo, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 11:02 ` Krzysztof Kozlowski @ 2025-02-08 4:21 ` Crystal Guo (郭晶) 2025-02-11 12:21 ` Crystal Guo (郭晶) 1 sibling, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-08 4:21 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 11:02 ` Krzysztof Kozlowski 2025-02-08 4:21 ` Crystal Guo (郭晶) @ 2025-02-11 12:21 ` Crystal Guo (郭晶) 1 sibling, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-11 12:21 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo 2024-12-12 10:46 ` AngeloGioacchino Del Regno 2024-12-12 11:02 ` Krzysztof Kozlowski @ 2024-12-21 1:42 ` kernel test robot 2024-12-21 11:03 ` kernel test robot 3 siblings, 0 replies; 25+ messages in thread From: kernel test robot @ 2024-12-21 1:42 UTC (permalink / raw) To: Crystal Guo, Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno Cc: oe-kbuild-all, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo ` (2 preceding siblings ...) 2024-12-21 1:42 ` kernel test robot @ 2024-12-21 11:03 ` kernel test robot 3 siblings, 0 replies; 25+ messages in thread From: kernel test robot @ 2024-12-21 11:03 UTC (permalink / raw) To: Crystal Guo, Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno Cc: llvm, oe-kbuild-all, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 8:59 [PATCH 0/2] Add MediaTek DRAMC driver support Crystal Guo 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo @ 2024-12-12 8:59 ` Crystal Guo 2024-12-12 10:27 ` Rob Herring (Arm) ` (2 more replies) 1 sibling, 3 replies; 25+ messages in thread From: Crystal Guo @ 2024-12-12 8:59 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno, Crystal Guo Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 8:59 ` [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings Crystal Guo @ 2024-12-12 10:27 ` Rob Herring (Arm) 2025-02-08 4:24 ` Crystal Guo (郭晶) 2024-12-12 10:27 ` AngeloGioacchino Del Regno 2024-12-12 10:58 ` Krzysztof Kozlowski 2 siblings, 1 reply; 25+ messages in thread From: Rob Herring (Arm) @ 2024-12-12 10:27 UTC (permalink / raw) To: Crystal Guo Cc: Project_Global_Chrome_Upstream_Group, Matthias Brugger, AngeloGioacchino Del Regno, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, linux-mediatek, Conor Dooley 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 10:27 ` Rob Herring (Arm) @ 2025-02-08 4:24 ` Crystal Guo (郭晶) 0 siblings, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-08 4:24 UTC (permalink / raw) To: robh@kernel.org Cc: linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, krzk@kernel.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group, conor+dt@kernel.org, linux-arm-kernel@lists.infradead.org, matthias.bgg@gmail.com, AngeloGioacchino Del Regno 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 8:59 ` [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings Crystal Guo 2024-12-12 10:27 ` Rob Herring (Arm) @ 2024-12-12 10:27 ` AngeloGioacchino Del Regno 2025-02-08 4:33 ` Crystal Guo (郭晶) 2025-02-11 12:46 ` Crystal Guo (郭晶) 2024-12-12 10:58 ` Krzysztof Kozlowski 2 siblings, 2 replies; 25+ messages in thread From: AngeloGioacchino Del Regno @ 2024-12-12 10:27 UTC (permalink / raw) To: Crystal Guo, Krzysztof Kozlowski, Rob Herring, Conor Dooley, Matthias Brugger Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 10:27 ` AngeloGioacchino Del Regno @ 2025-02-08 4:33 ` Crystal Guo (郭晶) 2025-02-11 12:46 ` Crystal Guo (郭晶) 1 sibling, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-08 4:33 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, AngeloGioacchino Del Regno, krzk@kernel.org, conor+dt@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 10:27 ` AngeloGioacchino Del Regno 2025-02-08 4:33 ` Crystal Guo (郭晶) @ 2025-02-11 12:46 ` Crystal Guo (郭晶) 1 sibling, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-11 12:46 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, AngeloGioacchino Del Regno, krzk@kernel.org, conor+dt@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 8:59 ` [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings Crystal Guo 2024-12-12 10:27 ` Rob Herring (Arm) 2024-12-12 10:27 ` AngeloGioacchino Del Regno @ 2024-12-12 10:58 ` Krzysztof Kozlowski 2025-02-08 4:40 ` Crystal Guo (郭晶) 2025-02-11 12:56 ` Crystal Guo (郭晶) 2 siblings, 2 replies; 25+ messages in thread From: Krzysztof Kozlowski @ 2024-12-12 10:58 UTC (permalink / raw) To: Crystal Guo, Rob Herring, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 10:58 ` Krzysztof Kozlowski @ 2025-02-08 4:40 ` Crystal Guo (郭晶) 2025-02-09 10:52 ` Krzysztof Kozlowski 2025-02-11 12:56 ` Crystal Guo (郭晶) 1 sibling, 1 reply; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-08 4:40 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2025-02-08 4:40 ` Crystal Guo (郭晶) @ 2025-02-09 10:52 ` Krzysztof Kozlowski 2025-02-11 12:59 ` Crystal Guo (郭晶) 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2025-02-09 10:52 UTC (permalink / raw) To: Crystal Guo (郭晶), robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2025-02-09 10:52 ` Krzysztof Kozlowski @ 2025-02-11 12:59 ` Crystal Guo (郭晶) 2025-02-11 15:57 ` Krzysztof Kozlowski 0 siblings, 1 reply; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-11 12:59 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2025-02-11 12:59 ` Crystal Guo (郭晶) @ 2025-02-11 15:57 ` Krzysztof Kozlowski 2025-02-12 3:21 ` Crystal Guo (郭晶) 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2025-02-11 15:57 UTC (permalink / raw) To: Crystal Guo (郭晶), robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2025-02-11 15:57 ` Krzysztof Kozlowski @ 2025-02-12 3:21 ` Crystal Guo (郭晶) 0 siblings, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-12 3:21 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2024-12-12 10:58 ` Krzysztof Kozlowski 2025-02-08 4:40 ` Crystal Guo (郭晶) @ 2025-02-11 12:56 ` Crystal Guo (郭晶) 2025-02-11 15:55 ` Krzysztof Kozlowski 1 sibling, 1 reply; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-11 12:56 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2025-02-11 12:56 ` Crystal Guo (郭晶) @ 2025-02-11 15:55 ` Krzysztof Kozlowski 2025-02-12 2:47 ` Crystal Guo (郭晶) 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2025-02-11 15:55 UTC (permalink / raw) To: Crystal Guo (郭晶), robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings 2025-02-11 15:55 ` Krzysztof Kozlowski @ 2025-02-12 2:47 ` Crystal Guo (郭晶) 0 siblings, 0 replies; 25+ messages in thread From: Crystal Guo (郭晶) @ 2025-02-12 2:47 UTC (permalink / raw) To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org, krzk@kernel.org, AngeloGioacchino Del Regno Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2025-02-12 3:21 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-12 8:59 [PATCH 0/2] Add MediaTek DRAMC driver support Crystal Guo 2024-12-12 8:59 ` [PATCH 1/2] memory/mediatek: Add an interface to get current DDR data rate Crystal Guo 2024-12-12 10:46 ` AngeloGioacchino Del Regno 2025-02-08 4:14 ` Crystal Guo (郭晶) 2025-02-11 12:13 ` Crystal Guo (郭晶) 2024-12-12 11:02 ` Krzysztof Kozlowski 2025-02-08 4:21 ` Crystal Guo (郭晶) 2025-02-11 12:21 ` Crystal Guo (郭晶) 2024-12-21 1:42 ` kernel test robot 2024-12-21 11:03 ` kernel test robot 2024-12-12 8:59 ` [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings Crystal Guo 2024-12-12 10:27 ` Rob Herring (Arm) 2025-02-08 4:24 ` Crystal Guo (郭晶) 2024-12-12 10:27 ` AngeloGioacchino Del Regno 2025-02-08 4:33 ` Crystal Guo (郭晶) 2025-02-11 12:46 ` Crystal Guo (郭晶) 2024-12-12 10:58 ` Krzysztof Kozlowski 2025-02-08 4:40 ` Crystal Guo (郭晶) 2025-02-09 10:52 ` Krzysztof Kozlowski 2025-02-11 12:59 ` Crystal Guo (郭晶) 2025-02-11 15:57 ` Krzysztof Kozlowski 2025-02-12 3:21 ` Crystal Guo (郭晶) 2025-02-11 12:56 ` Crystal Guo (郭晶) 2025-02-11 15:55 ` Krzysztof Kozlowski 2025-02-12 2:47 ` Crystal Guo (郭晶)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).