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

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

[kernel test robot]

:::::: 
:::::: Manual check reason: "dtcheck: binding changes may go via different trees"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20241212090029.13692-3-crystal.guo@mediatek.com>
References: <20241212090029.13692-3-crystal.guo@mediatek.com>
TO: Crystal Guo <crystal.guo@mediatek.com>
TO: Krzysztof Kozlowski <krzk@kernel.org>
TO: Rob Herring <robh@kernel.org>
TO: Conor Dooley <conor+dt@kernel.org>
TO: Matthias Brugger <matthias.bgg@gmail.com>
TO: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
TO: Crystal Guo <crystal.guo@mediatek.com>
CC: linux-kernel@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
CC: linux-mediatek@lists.infradead.org
CC: Project_Global_Chrome_Upstream_Group@mediatek.com

Hi Crystal,

kernel test robot noticed the following build warnings:

[auto build test WARNING on krzk-mem-ctrl/for-next]
[also build test WARNING 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-3-crystal.guo%40mediatek.com
patch subject: [PATCH 2/2] dt-bindings: memory-controllers: Add mediatek common-dramc dt-bindings
:::::: branch date: 8 days ago
:::::: commit date: 8 days ago
config: csky-randconfig-052-20241220 (https://download.01.org/0day-ci/archive/20241220/202412202152.7WyYu0Y6-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
dtschema version: 2024.12.dev5+gea243eb
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241220/202412202152.7WyYu0Y6-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/r/202412202152.7WyYu0Y6-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: support-ch-cnt: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: fmeter-version: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: crystal-freq: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: shu-of: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: pll-id: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: shu-lv: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: sdmpcw: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: posdiv: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: fbksel: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: dqsopen: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: async-ca: missing type definition
>> Documentation/devicetree/bindings/memory-controllers/mediatek,common-dramc.yaml: dq-ser-mode: missing type definition
   Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition

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

[PATCH 0/2] Add MediaTek DRAMC driver support

[Crystal Guo]

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

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

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

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

-- 
2.18.0

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

[Crystal Guo]

Add devicetree binding for mediatek common-dramc driver.

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

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

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

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

[Rob Herring (Arm)]

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

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

yamllint warnings/errors:

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

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

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

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

[Crystal Guo (郭晶)]

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

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

Thanks
Crystal

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

[AngeloGioacchino Del Regno]

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

MediaTek DRAM Controller (DRAMC)

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

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

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

Absolutely no! Compatibles are per-soc.

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

etc

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

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

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

So the reg can be simplified as

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


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

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

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

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

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

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

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

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

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

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

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

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

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

Besides, I doubt that those are board specific.

Regards,
Angelo

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

[Crystal Guo (郭晶)]

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

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

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

Thanks
Crystal

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

[Crystal Guo (郭晶)]

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

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

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

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

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

Change the compatible to "mediatek,mt8196-dramc"

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

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

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

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

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

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

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

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

Thanks
Crystal

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

[Krzysztof Kozlowski]

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

Bindings are before users.



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

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

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

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

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

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

So not common here?

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

This has to be SoC.

> +
> +  reg:
> +    minItems: 9

Why this is flexible?

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

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

> +
> +  support-ch-cnt:

Nope

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


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


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

memory-controller@
and drop unused label.

Best regards,
Krzysztof

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

[Crystal Guo (郭晶)]

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

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

Thanks
Crystal

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

[Krzysztof Kozlowski]

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

Do not ignore comments but implement them or keep discussing.

Best regards,
Krzysztof

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

[Crystal Guo (郭晶)]

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

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

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

[Krzysztof Kozlowski]

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


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

I assume above also means you implement the feedback :)

Best regards,
Krzysztof

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

[Crystal Guo (郭晶)]

On Tue, 2025-02-11 at 16: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

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

[Crystal Guo (郭晶)]

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

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

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


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

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

Title: MediaTek DRAM Controller (DRAMC)

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

Okay, remove it in v2.

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

Sorry, my original title description was not accurate.

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

Change to "mediatek,mt8196-dramc"

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

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

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

This part has been updated in v2 to:

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

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

Okay, remove this property.

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

Okay, thanks.
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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


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

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

Best regards,
Krzysztof

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

[Crystal Guo (郭晶)]

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

Okay, got it, thanks.

Best regards,
Crystal