Re: [PATCH 3/3] dt-bindings: mmc: dw-mshc-hi3798cv200: rename to dw-mshc-histb

[Yang Xiwen]

On 2/16/2024 4:21 PM, Krzysztof Kozlowski wrote:
> On 15/02/2024 18:46, Yang Xiwen via B4 Relay wrote:
> > From: Yang Xiwen <forbidden405@xxxxxxxxxxx>
> >
> > Add binding for Hi3798MV200 DWMMC specific extension.
> >
> > Signed-off-by: Yang Xiwen <forbidden405@xxxxxxxxxxx>
> > ---
> >   ...hi3798cv200-dw-mshc.yaml => histb-dw-mshc.yaml} | 60 +++++++++++++++++++---
> >   1 file changed, 52 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/hi3798cv200-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/histb-dw-mshc.yaml
> > similarity index 57%
> > rename from Documentation/devicetree/bindings/mmc/hi3798cv200-dw-mshc.yaml
> > rename to Documentation/devicetree/bindings/mmc/histb-dw-mshc.yaml
> > index 5db99cd94b90..d2f5b7bb7a58 100644
> > --- a/Documentation/devicetree/bindings/mmc/hi3798cv200-dw-mshc.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/histb-dw-mshc.yaml
> > @@ -1,11 +1,11 @@
> >   # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >   %YAML 1.2
> >   ---
> > -$id: http://devicetree.org/schemas/mmc/hi3798cv200-dw-mshc.yaml#
> > +$id: http://devicetree.org/schemas/mmc/histb-dw-mshc.yaml#
> Really, one wrong filename into another...
How about "hisilicon,dw-mshc.yaml"? I found rockchip using a similar 
naming: "rockchip-dw-mshc.yaml"
> >   $schema: http://devicetree.org/meta-schemas/core.yaml#
> >   
> >   title:
> > -  Hisilicon Hi3798CV200 SoC specific extensions to the Synopsys DWMMC controller
> > +  Hisilicon HiSTB SoCs specific extensions to the Synopsys DWMMC controller
> >   
> >   maintainers:
> >     - Yang Xiwen <forbidden405@xxxxxxxxxxx>
> > @@ -14,16 +14,14 @@ description:
> >     The Synopsys designware mobile storage host controller is used to interface
> >     a SoC with storage medium such as eMMC or SD/MMC cards. This file documents
> >     differences between the core Synopsys dw mshc controller properties described
> > -  by synopsys-dw-mshc.txt and the properties used by the Hisilicon Hi3798CV200
> > -  specific extensions to the Synopsys Designware Mobile Storage Host Controller.
> Just drop this sentence in previous/conversion patch. It's useless.
Will do in v2.
> > -
> > -allOf:
> > -  - $ref: synopsys-dw-mshc-common.yaml#
> Put it in correct place in the first time. Don't needlessly shuffle the
> code right after previous patch.
Will fix in v2.
> > +  by synopsys-dw-mshc.txt and the properties used by the Hisilicon HiSTB specific
> > +  extensions to the Synopsys Designware Mobile Storage Host Controller.
> >   
> >   properties:
> >     compatible:
> >       enum:
> >         - hisilicon,hi3798cv200-dw-mshc
> > +      - hisilicon,hi3798mv200-dw-mshc
> >   
> >     reg:
> >       maxItems: 1
> > @@ -48,6 +46,12 @@ properties:
> >         control the clock phases, "ciu-sample" is required for tuning
> >         high speed modes.
> >   
> > +  hisilicon,sap-dll-reg:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      A phandle points to the sample delay-locked-loop(DLL)
> > +      syscon node, used for tuning.
> Does hi3798cv200 have it?
No it does not. Currently only hi3798mv200 has it (it's called himci 
v300 in downstream, while cv200 is using himci v200).
> > +
> >   required:
> >     - compatible
> >     - reg
> > @@ -55,13 +59,25 @@ required:
> >     - clocks
> >     - clock-names
> >   
> > +allOf:
> > +  - $ref: synopsys-dw-mshc-common.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: hisilicon,hi3798mv200-dw-mshc
> > +    then:
> > +      required:
> > +        - hisilicon,sap-dll-reg
> > +
> >   unevaluatedProperties: false
> >   
> >   examples:
> >     - |
> >       #include <dt-bindings/clock/histb-clock.h>
> >       #include <dt-bindings/interrupt-controller/arm-gic.h>
> > -    emmc: mmc@9830000 {
> > +    mmc@9830000 {
> ???
It's complaining about duplicated label when i added emmc label to both 
nodes. I'll remove it in previous patch in v2.
> >         compatible = "hisilicon,hi3798cv200-dw-mshc";
> >         reg = <0x9830000 0x10000>;
> >         interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
> > @@ -84,3 +100,31 @@ examples:
> >         bus-width = <8>;
> >         status = "okay";
> >       };
> > +  - |
> > +    #include <dt-bindings/clock/histb-clock.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    mmc@9830000 {
> > +      compatible = "hisilicon,hi3798mv200-dw-mshc";
> No need for new example.
>
> > +      reg = <0x9830000 0x10000>;
> > +      interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
> > +      clocks = <&crg HISTB_MMC_CIU_CLK>,
> > +               <&crg HISTB_MMC_BIU_CLK>,
> > +               <&crg HISTB_MMC_SAMPLE_CLK>,
> > +               <&crg HISTB_MMC_DRV_CLK>;
> > +      clock-names = "ciu", "biu", "ciu-sample", "ciu-drive";
> > +      resets = <&crg 0xa0 4>;
> > +      reset-names = "reset";
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&emmc_pins>;
> > +      fifo-depth = <256>;
> > +      clock-frequency = <50000000>;
> > +      max-frequency = <150000000>;
> > +      cap-mmc-highspeed;
> > +      mmc-ddr-1_8v;
> > +      mmc-hs200-1_8v;
> > +      mmc-hs400-1_8v;
> > +      non-removable;
> > +      bus-width = <8>;
> > +      hisilicon,sap-dll-reg = <&emmc_sap_dll_reg>;
> > +      status = "okay";
> No, really...
The property "hisilicon,sap-dll-reg" is introduced in this patch, i want 
to add an example for it here since the common dtsi will use this 
binding and will be submitted when it gets ready.
> > +    };
> Best regards,
> Krzysztof

--
Regards,
Yang Xiwen