diff mbox series

[2/6] dt-bindings: mmc: Add support for BCM2712 SD host controller

Message ID ad96fff723675c2d65a5e3328da9b09f2781cbcd.1713036964.git.andrea.porta@suse.com (mailing list archive)
State New
Headers show
Series Add support for BCM2712 SD card controller | expand

Commit Message

Andrea della Porta April 13, 2024, 10:14 p.m. UTC
Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 51 ++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Comments

Rob Herring (Arm) April 13, 2024, 11:22 p.m. UTC | #1
On Sun, 14 Apr 2024 00:14:24 +0200, Andrea della Porta wrote:
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>  .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 51 ++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml:91:10: [warning] wrong indentation: expected 10 but found 9 (indentation)
./Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml:91:17: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/ad96fff723675c2d65a5e3328da9b09f2781cbcd.1713036964.git.andrea.porta@suse.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.
Krzysztof Kozlowski April 14, 2024, 6:11 a.m. UTC | #2
On 14/04/2024 00:14, Andrea della Porta wrote:
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>

? And what is being done here and why?

> ---
>  .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 51 ++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> index cbd3d6c6c77f..6aa137d78e4f 100644
> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> @@ -13,6 +13,7 @@ maintainers:
>  properties:
>    compatible:
>      oneOf:
> +      - const: brcm,bcm2712-sdhci
>        - items:
>            - enum:
>                - brcm,bcm7216-sdhci
> @@ -26,12 +27,16 @@ properties:
>            - const: brcm,sdhci-brcmstb
>  
>    reg:
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 4
>  
>    reg-names:
> +    minItems: 2
>      items:
>        - const: host
>        - const: cfg
> +      - const: busisol
> +      - const: lcpll
>  
>    interrupts:
>      maxItems: 1
> @@ -60,6 +65,7 @@ properties:
>      description: Specifies that controller should use auto CMD12
>  
>  allOf:
> +  - $ref: sdhci-common.yaml
>    - $ref: mmc-controller.yaml#

Why? Anyway, this replaces mmc-controller, doesn't it?

>    - if:
>        properties:
> @@ -71,6 +77,28 @@ allOf:
>        required:
>          - clock-frequency
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm2712-sdhci
> +
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 4
> +        clock-names:
> +         const: "sw_sdio"

Not tested.

> +
> +    else:
> +      properties:
> +        reg:
> +          minItems: 2
> +          maxItems: 2
> +        reg-names:
> +          minItems: 2
> +          maxItems: 2
> +
>  required:
>    - compatible
>    - reg
> @@ -114,3 +142,24 @@ examples:
>        clocks = <&scmi_clk 245>;
>        clock-names = "sw_sdio";
>      };
> +
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      mmc@fff000 {
> +        compatible = "brcm,bcm2712-sdhci";
> +        reg = <0x10 0x00fff000  0x0 0x260>,
> +              <0x10 0x00fff400  0x0 0x200>,
> +              <0x10 0x015040b0  0x0 0x4>,  // Bus isolation control
> +              <0x10 0x015200f0  0x0 0x24>; // LCPLL control misc0-8
> +        reg-names = "host", "cfg", "busisol", "lcpll";
> +        interrupts = <0x0 0x111 0x4>;

Use proper defines.

> +        clocks = <&clk_emmc2>;
> +        sdhci-caps-mask = <0x0000C000 0x0>;
> +        sdhci-caps = <0x0 0x0>;
> +        mmc-ddr-3_3v;
> +        clock-names = "sw_sdio";

names *always* follow property. In every DTS. Please fix youro DTS.



Best regards,
Krzysztof
Florian Fainelli April 14, 2024, 3:55 p.m. UTC | #3
On 4/13/2024 3:14 PM, Andrea della Porta wrote:
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>   .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 51 ++++++++++++++++++-
>   1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> index cbd3d6c6c77f..6aa137d78e4f 100644
> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> @@ -13,6 +13,7 @@ maintainers:
>   properties:
>     compatible:
>       oneOf:
> +      - const: brcm,bcm2712-sdhci
>         - items:
>             - enum:
>                 - brcm,bcm7216-sdhci
> @@ -26,12 +27,16 @@ properties:
>             - const: brcm,sdhci-brcmstb
>   
>     reg:
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 4
>   
>     reg-names:
> +    minItems: 2
>       items:
>         - const: host
>         - const: cfg
> +      - const: busisol
> +      - const: lcpll
>   
>     interrupts:
>       maxItems: 1
> @@ -60,6 +65,7 @@ properties:
>       description: Specifies that controller should use auto CMD12
>   
>   allOf:
> +  - $ref: sdhci-common.yaml
>     - $ref: mmc-controller.yaml#
>     - if:
>         properties:
> @@ -71,6 +77,28 @@ allOf:
>         required:
>           - clock-frequency
>   
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm2712-sdhci
> +
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 4
> +        clock-names:
> +         const: "sw_sdio"
> +
> +    else:
> +      properties:
> +        reg:
> +          minItems: 2
> +          maxItems: 2
> +        reg-names:
> +          minItems: 2
> +          maxItems: 2
> +
>   required:
>     - compatible
>     - reg
> @@ -114,3 +142,24 @@ examples:
>         clocks = <&scmi_clk 245>;
>         clock-names = "sw_sdio";
>       };
> +
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      mmc@fff000 {
> +        compatible = "brcm,bcm2712-sdhci";
> +        reg = <0x10 0x00fff000  0x0 0x260>,
> +              <0x10 0x00fff400  0x0 0x200>,
> +              <0x10 0x015040b0  0x0 0x4>,  // Bus isolation control

That should be a syscon, not under direct management of the driver 
because there are other bits in that register that are relevant here.

> +              <0x10 0x015200f0  0x0 0x24>; // LCPLL control misc0-8

And likewise, this should be a clock provider, or the firmware could add 
support for configuring the LCPLL since there are other things going on 
there.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
index cbd3d6c6c77f..6aa137d78e4f 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
@@ -13,6 +13,7 @@  maintainers:
 properties:
   compatible:
     oneOf:
+      - const: brcm,bcm2712-sdhci
       - items:
           - enum:
               - brcm,bcm7216-sdhci
@@ -26,12 +27,16 @@  properties:
           - const: brcm,sdhci-brcmstb
 
   reg:
-    maxItems: 2
+    minItems: 2
+    maxItems: 4
 
   reg-names:
+    minItems: 2
     items:
       - const: host
       - const: cfg
+      - const: busisol
+      - const: lcpll
 
   interrupts:
     maxItems: 1
@@ -60,6 +65,7 @@  properties:
     description: Specifies that controller should use auto CMD12
 
 allOf:
+  - $ref: sdhci-common.yaml
   - $ref: mmc-controller.yaml#
   - if:
       properties:
@@ -71,6 +77,28 @@  allOf:
       required:
         - clock-frequency
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm2712-sdhci
+
+    then:
+      properties:
+        reg:
+          maxItems: 4
+        clock-names:
+         const: "sw_sdio"
+
+    else:
+      properties:
+        reg:
+          minItems: 2
+          maxItems: 2
+        reg-names:
+          minItems: 2
+          maxItems: 2
+
 required:
   - compatible
   - reg
@@ -114,3 +142,24 @@  examples:
       clocks = <&scmi_clk 245>;
       clock-names = "sw_sdio";
     };
+
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      mmc@fff000 {
+        compatible = "brcm,bcm2712-sdhci";
+        reg = <0x10 0x00fff000  0x0 0x260>,
+              <0x10 0x00fff400  0x0 0x200>,
+              <0x10 0x015040b0  0x0 0x4>,  // Bus isolation control
+              <0x10 0x015200f0  0x0 0x24>; // LCPLL control misc0-8
+        reg-names = "host", "cfg", "busisol", "lcpll";
+        interrupts = <0x0 0x111 0x4>;
+        clocks = <&clk_emmc2>;
+        sdhci-caps-mask = <0x0000C000 0x0>;
+        sdhci-caps = <0x0 0x0>;
+        mmc-ddr-3_3v;
+        clock-names = "sw_sdio";
+      };
+    };