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