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@xxxxxxxxxxxx> > > --- > > .../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@xxxxxxxxxxxx> > > + > > +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
