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.0On 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
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
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
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
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
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
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.
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
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
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
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
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
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.
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
© 2016 - 2025 Red Hat, Inc.