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