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

From: Crystal Guo (郭晶)
Date: Wed Apr 09 2025 - 03:42:53 EST

Next message: Boris Brezillon: "Re: [PATCH v5 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS"
Previous message: Oscar Salvador: "Re: [PATCH] mm/hugetlb: use separate nodemask for bootmem allocations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2025-04-04 at 13:11 +0200, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Please use subject prefixes matching the subsystem. You can get them
> for
> example with 'git log --oneline -- DIRECTORY_OR_FILE' on the
> directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
>
https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html*i-for-patch-submitters__;Iw!!CTRNKA9wMg0ARbw!i9YnJLxKI_KuaW2pzwPR_GjZT8IzcZVycscosWKhQjWxvSouP0La_YLNk6lFRLpwDSUbgg8EGbZjbOjX$
>
> memory: mediatek:

Thanks for the suggestion, I will use the correct subject prefixes in
the next version:
'memory: mediatek: Add an interface to get current DDR data rate'

>
> On Thu, Apr 03, 2025 at 02:48:48PM GMT, 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 | 20 +++
> > drivers/memory/mediatek/Makefile | 2 +
> > drivers/memory/mediatek/mtk-dramc.c | 223
> > ++++++++++++++++++++++++++++
> > 5 files changed, 247 insertions(+)
> > create mode 100644 drivers/memory/mediatek/Kconfig
> > create mode 100644 drivers/memory/mediatek/Makefile
> > create mode 100644 drivers/memory/mediatek/mtk-dramc.c
> >
> > 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"
>
> m goes before s, so this goes before samsung source.
>

Okay, I will update it in the next version.

> >
> > endif
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index d2e6ca9abbe0..c0facf529803 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_MEDIATEK_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..eadc11ec0f1c
> > --- /dev/null
> > +++ b/drivers/memory/mediatek/Kconfig
> > @@ -0,0 +1,20 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config MEDIATEK_MC
> > + bool "MediaTek Memory Controller support"
> > + default y
>
> Why this has to be enabled for every compile test build? Look how
> other
> platforms do it.
>
> I'll fix the tegra.
>

Okay, I will change 'default Y' to 'default ARCH_MEDIATEK' in the next
version.

>
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + help
> > + This option allows to enable MediaTek memory controller
> > drivers,
> > + which may include controllers for DRAM or others.
> > +
> > +if MEDIATEK_MC
> > +
> > +config MTK_DRAMC
> > + tristate "MediaTek DRAMC driver"
> > + default y
>
> This matters less, could stay or could be also ARCH_MDIATEK.
>

Understood, I will keep this part unchanged in the next version.
Thank you for your suggestion.

> > + help
> > + This driver is for the DRAM Controller found in MediaTek
> > SoCs
> > + and provides a sysfs interface for reporting the current
> > DRAM
> > + data rate.
> > +
> > +endif
>
> ...
>
> > +
> > +static unsigned int read_reg_field(void __iomem *base, unsigned
> > int offset, unsigned int mask)
> > +{
> > + unsigned int val = readl(base + offset);
> > + unsigned int shift = __ffs(mask);
> > +
> > + return (val & mask) >> shift;
> > +}
> > +
> > +static int mtk_dramc_probe(struct platform_device *pdev)
>
> Weird ordering. probe() is one of the last functions. Only other
> driver
> struct functions (like remove, suspend/resume) go after, not some
> regular code.

Understood. I will adjust the order of the probe function in the next
version.

>
> > +{
> > + struct mtk_dramc *dramc;
> > + const struct mtk_dramc_pdata *pdata;
> > +
> > + dramc = devm_kzalloc(&pdev->dev, sizeof(struct mtk_dramc),
> > GFP_KERNEL);
> > + if (!dramc)
> > + return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to
> > allocate memory\n");
> > +
> > + pdata = of_device_get_match_data(&pdev->dev);
> > + if (!pdata)
> > + return dev_err_probe(&pdev->dev, -EINVAL, "No
> > platform data available\n");
>
> Just return. That's impossible condition, so no need for printing
> errors.

Okay, will return directly in the next version.

>
> > +
> > + dramc->pdata = pdata;
>
> Why do you need pdata variable in the first place? Make this code
> simple, not complicated.

Okay, will remove redundant 'pdata' in the next version.

>
> > +
> > + dramc->anaphy_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(dramc->anaphy_base))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(dramc-
> > >anaphy_base),
> > + "Unable to map ANAPHY base\n");
> > +
> > + dramc->ddrphy_base = devm_platform_ioremap_resource(pdev, 1);
> > + if (IS_ERR(dramc->ddrphy_base))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(dramc-
> > >ddrphy_base),
> > + "Unable to map DDRPHY base\n");
> > +
> > + platform_set_drvdata(pdev, dramc);
> > + return 0;
> > +}
> > +
> > +static unsigned int mtk_fmeter_v1(struct mtk_dramc *dramc)
> > +{
> > + const struct mtk_dramc_pdata *pdata = dramc->pdata;
> > + unsigned int shu_level, pll_sel, offset;
> > + unsigned int sdmpcw, posdiv, clkdiv, fbksel, sopen, async_ca,
> > ser_mode;
> > + unsigned int prediv_freq, posdiv_freq, vco_freq;
> > + unsigned int final_rate;
> > +
> > + shu_level = read_reg_field(dramc->ddrphy_base, pdata-
> > >regs[DRAMC_DPHY_DVFS_STA],
> > + pdata-
> > >masks[DRAMC_DPHY_DVFS_SHU_LV]);
>
> Don't creat your own wrappers. Use existing FIELD_PREP stuff.

Since the mask is not a constant, using FIELD_GET will result in a
compilation error. Can I keep using 'read_reg_field' in the next
version?

>
> > + pll_sel = read_reg_field(dramc->ddrphy_base, pdata-
> > >regs[DRAMC_DPHY_DVFS_STA],
> > + pdata-
> > >masks[DRAMC_DPHY_DVFS_PLL_SEL]);
> > + offset = pdata->shuffle_offset * shu_level;
> > +
> > + sdmpcw = read_reg_field(dramc->anaphy_base,
> > + ((pll_sel == 0) ?
> > + pdata->regs[DRAMC_APHY_SHU_PHYPLL2] :
> > + pdata->regs[DRAMC_APHY_SHU_CLRPLL2])
> > + offset,
> > + pdata-
> > >masks[DRAMC_APHY_PLL2_SDMPCW]);
> > + posdiv = read_reg_field(dramc->anaphy_base,
> > + ((pll_sel == 0) ?
> > + pdata->regs[DRAMC_APHY_SHU_PHYPLL3] :
> > + pdata->regs[DRAMC_APHY_SHU_CLRPLL3])
> > + offset,
> > + pdata-
> > >masks[DRAMC_APHY_PLL3_POSDIV]);
> > + fbksel = read_reg_field(dramc->anaphy_base, pdata-
> > >regs[DRAMC_APHY_SHU_PHYPLL4] + offset,
> > + pdata-
> > >masks[DRAMC_APHY_PLL4_FBKSEL]);
> > + sopen = read_reg_field(dramc->anaphy_base, pdata-
> > >regs[DRAMC_APHY_ARPI0] + offset,
> > + pdata->masks[DRAMC_APHY_ARPI0_SOPEN]);
> > + async_ca = read_reg_field(dramc->anaphy_base, pdata-
> > >regs[DRAMC_APHY_CA_ARDLL1] + offset,
> > + pdata-
> > >masks[DRAMC_APHY_ARDLL1_CK_EN]);
> > + ser_mode = read_reg_field(dramc->anaphy_base, pdata-
> > >regs[DRAMC_APHY_B0_TX0] + offset,
> > + pdata-
> > >masks[DRAMC_APHY_B0_TX0_SER_MODE]);
> > +
> > + clkdiv = (ser_mode == 1) ? 1 : 0;
> > + posdiv &= ~(pdata->posdiv_purify);
> > +
> > + prediv_freq = pdata->ref_freq_mhz * (sdmpcw >> pdata-
> > >prediv);
> > + posdiv_freq = (prediv_freq >> posdiv) >> 1;
> > + vco_freq = posdiv_freq << fbksel;
> > + final_rate = vco_freq >> clkdiv;
> > +
> > + if (sopen == 1 && async_ca == 1)
> > + final_rate >>= 1;
> > +
> > + return final_rate;
> > +}
> > +
> > +/**
> > + * mtk_dramc_get_data_rate - Calculate the current DRAM data rate
> > + * @dev: Device pointer
> > + *
> > + * Return: DRAM Data Rate in Mbps or negative number for error
> > + */
> > +static unsigned int mtk_dramc_get_data_rate(struct device *dev)
> > +{
> > + struct mtk_dramc *dramc = dev_get_drvdata(dev);
> > +
> > + if (dramc->pdata->fmeter_version == 1)
>
> Drop, it's not possible to have other case.

Thank you for your suggestion.
In the future, other SoCs might use different fmeter version to
calculate the DDR data rate. If we remove the fmeter_version now,
future SoCs that cannot share mtk_fmeter_v1 will still need to add the
fmeter_version variable.
Therefore, can I keep the current implementation for now.

>
> > + return mtk_fmeter_v1(dramc);
> > +
> > + dev_err(dev, "Frequency meter version %u not supported\n",
> > dramc->pdata->fmeter_version);
> > + return -EINVAL;
> > +}
> > +
> > +static ssize_t dram_data_rate_show(struct device *dev,
> > + struct device_attribute *attr,
> > char *buf)
>
> No ABI doc.
>
> Why existing interconnect interface is not suitable here? This
> should
> be an interconnect, otherwise what is the point of this driver? What
> do
> you exactly configure here?
>

This driver specifically focuses on displaying the current DDR data
rate, which is used for the DVFS (Dynamic Voltage and Frequency
Scaling) feature to verify that DDR frequency adjustments are correctly
executed.
I will add the ABI documentation for the dram_data_rate attribute as
follows, is this ok?

What: /sys/devices/platform/soc/XXXXXXXX.memory-
controller/dram_data_rate
Date: April 2025
KernelVersion: 6.14
Contact: Crystal Guo <crystal.guo@xxxxxxxxxxxx>
Description:
Reports the current DRAM data rate in Mbps.

Access: Read

>
> > +{
> > + return snprintf(buf, PAGE_SIZE, "DRAM data rate = %u\n",
> > + mtk_dramc_get_data_rate(dev));
> > +}
> > +
> > +static DEVICE_ATTR_RO(dram_data_rate);
> > +
> > +static struct attribute *mtk_dramc_attrs[] = {
> > + &dev_attr_dram_data_rate.attr,
> > + NULL
> > +};
> > +ATTRIBUTE_GROUPS(mtk_dramc);
> > +
> > +static const struct mtk_dramc_pdata dramc_pdata_mt8196 = {
> > + .fmeter_version = 1,
> > + .ref_freq_mhz = 26,
> > + .regs = mtk_dramc_regs_mt8196,
> > + .masks = mtk_dramc_masks_mt8196,
> > + .posdiv_purify = BIT(2),
> > + .prediv = 7,
> > + .shuffle_offset = 0x700,
> > +};
> > +
> > +static const struct of_device_id mtk_dramc_of_ids[] = {
> > + { .compatible = "mediatek,mt8196-dramc", .data =
> > &dramc_pdata_mt8196 },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_dramc_of_ids);
> > +
> > +static struct platform_driver mtk_dramc_driver = {
> > + .probe = mtk_dramc_probe,
> > + .driver = {
> > + .name = "mtk-dramc",
> > + .of_match_table = mtk_dramc_of_ids,
> > + .dev_groups = mtk_dramc_groups,
> > + },
> > +};
> > +module_platform_driver(mtk_dramc_driver);
> > +
> > +MODULE_AUTHOR("Crystal Guo <crystal.guo@xxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("MediaTek DRAM Controller Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.18.0
> >

Next message: Boris Brezillon: "Re: [PATCH v5 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS"
Previous message: Oscar Salvador: "Re: [PATCH] mm/hugetlb: use separate nodemask for bootmem allocations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]