diff mbox series

[v9,02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC

Message ID 20230119035136.21603-3-blarson@amd.com
State New
Headers
Series Support AMD Pensando Elba SoC |

Commit Message

Brad Larson Jan. 19, 2023, 3:51 a.m. UTC 
  AMD Pensando Elba ARM 64-bit SoC is integrated with this IP and
explicitly controls byte-lane enables.

Signed-off-by: Brad Larson <blarson@amd.com>

---

Changes since v6:
- Add reset-names and resets properties
- Add if/then on property amd,pensando-elba-sd4hc to set reg property
  values for minItems and maxItems

---
 .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 28 ++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Krzysztof Kozlowski Jan. 19, 2023, 7:47 a.m. UTC | #1 
On 19/01/2023 04:51, Brad Larson wrote:
> AMD Pensando Elba ARM 64-bit SoC is integrated with this IP and
> explicitly controls byte-lane enables.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> 
> ---
> 
> Changes since v6:
> - Add reset-names and resets properties
> - Add if/then on property amd,pensando-elba-sd4hc to set reg property
>   values for minItems and maxItems
> 
> ---
>  .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 28 ++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index 8b1a0fdcb5e3..f7dd6f990f96 100644
> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> @@ -16,12 +16,14 @@ properties:
>    compatible:
>      items:
>        - enum:
> +          - amd,pensando-elba-sd4hc
>            - microchip,mpfs-sd4hc
>            - socionext,uniphier-sd4hc
>        - const: cdns,sd4hc
>  
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
>  
>    interrupts:
>      maxItems: 1
> @@ -111,12 +113,36 @@ properties:
>      minimum: 0
>      maximum: 0x7f
>  
> +  reset-names:
> +    items:
> +      - const: hw
> +
> +  resets:
> +    description:
> +      optional. phandle to the system reset controller with line index

Drop "optional"
Drop "phandle to the" and rephrase it to describe physical reset line.
Don't describe here DT syntax (phandle) but the hardware. What is
expected to be here?

> +      for mmc hw reset line if exists.
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
>    - clocks
>  
> +if:

Move the allO from the top here and put it under it. Saves indentation soon.

> +  properties:
> +    compatible:
> +      const: amd,pensando-elba-sd4hc
> +then:
> +  properties:
> +    reg:
> +      minItems: 2
> +else:
> +  properties:
> +    reg:
> +      minItems: 1
> +      maxItems: 2

No, why do you suddenly allow two items on all variants? This was not
described in your commit msg at all, so I expect here maxItems: 1.

Also, unless your reset is applicable to all variants, resets: false and
reset-names: false.

> +
>  unevaluatedProperties: false
>  
>  examples:

Best regards,
Krzysztof
Krzysztof Kozlowski Jan. 19, 2023, 7:48 a.m. UTC | #2 
On 19/01/2023 04:51, Brad Larson wrote:
> AMD Pensando Elba ARM 64-bit SoC is integrated with this IP and
> explicitly controls byte-lane enables.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> 
> ---
> 
> Changes since v6:
> - Add reset-names and resets properties
> - Add if/then on property amd,pensando-elba-sd4hc to set reg property
>   values for minItems and maxItems
> 
> ---
>  .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 28 ++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index 8b1a0fdcb5e3..f7dd6f990f96 100644
> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> @@ -16,12 +16,14 @@ properties:
>    compatible:
>      items:
>        - enum:
> +          - amd,pensando-elba-sd4hc
>            - microchip,mpfs-sd4hc
>            - socionext,uniphier-sd4hc
>        - const: cdns,sd4hc
>  
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
>  
>    interrupts:
>      maxItems: 1
> @@ -111,12 +113,36 @@ properties:
>      minimum: 0
>      maximum: 0x7f
>  
> +  reset-names:
> +    items:
> +      - const: hw
> +
> +  resets:
> +    description:
> +      optional. phandle to the system reset controller with line index
> +      for mmc hw reset line if exists.
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
>    - clocks
>  
> +if:
> +  properties:
> +    compatible:
> +      const: amd,pensando-elba-sd4hc

BTW, this probably won't even work and that's the answer why you added
fake maxItems: 2... This should make you think about the bug. You must
use contains.

Best regards,
Krzysztof
Brad Larson Jan. 21, 2023, 1:10 a.m. UTC | #3 
On 19/01/2023 7:47 UTC, Krzysztof Kozlowski wrote:
>On 19/01/2023 04:51, Brad Larson wrote:
>> AMD Pensando Elba ARM 64-bit SoC is integrated with this IP and
>> explicitly controls byte-lane enables.
>> 
...
>> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
>> index 8b1a0fdcb5e3..f7dd6f990f96 100644
>> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
>> @@ -16,12 +16,14 @@ properties:
>>    compatible:
>>      items:
>>        - enum:
>> +          - amd,pensando-elba-sd4hc
>>            - microchip,mpfs-sd4hc
>>            - socionext,uniphier-sd4hc
>>        - const: cdns,sd4hc
>>  
>>    reg:
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>    interrupts:
>>      maxItems: 1
>> @@ -111,12 +113,36 @@ properties:
>>      minimum: 0
>>      maximum: 0x7f
>>  
>> +  reset-names:
>> +    items:
>> +      - const: hw
>> +
>> +  resets:
>> +    description:
>> +      optional. phandle to the system reset controller with line index
>
>Drop "optional"
>Drop "phandle to the" and rephrase it to describe physical reset line.
>Don't describe here DT syntax (phandle) but the hardware. What is
>expected to be here?

Done, see the resulting diff below for full context.  The missing
'contains' was the bug.

>> +      for mmc hw reset line if exists.
>> +    maxItems: 1
>> +
>>  required:
>>    - compatible
>>    - reg
>>    - interrupts
>>    - clocks
>>  
>> +if:
>
>Move the allO from the top here and put it under it. Saves indentation soon.

Yes.

>> +  properties:
>> +    compatible:
>> +      const: amd,pensando-elba-sd4hc
>
>BTW, this probably won't even work and that's the answer why you added
>fake maxItems: 2... This should make you think about the bug. You must
>use contains.

That was the problem, see updated diff below.  Passes dtbs_check and dt_binding_check.

>> +then:
>> +  properties:
>> +    reg:
>> +      minItems: 2
>> +else:
>> +  properties:
>> +    reg:
>> +      minItems: 1
>> +      maxItems: 2
>
>No, why do you suddenly allow two items on all variants? This was not
>described in your commit msg at all, so I expect here maxItems: 1.

Set maxItems: 1.

>Also, unless your reset is applicable to all variants, resets: false and
>reset-names: false.

Added false for both, this is the diff with above changes

--- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
@@ -9,19 +9,18 @@ title: Cadence SD/SDIO/eMMC Host Controller (SD4HC)
 maintainers:
   - Masahiro Yamada <yamada.masahiro@socionext.com>
 
-allOf:
-  - $ref: mmc-controller.yaml
-
 properties:
   compatible:
     items:
       - enum:
+          - amd,pensando-elba-sd4hc
           - microchip,mpfs-sd4hc
           - socionext,uniphier-sd4hc
       - const: cdns,sd4hc
 
   reg:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   interrupts:
     maxItems: 1
@@ -111,12 +110,42 @@ properties:
     minimum: 0
     maximum: 0x7f
 
+  reset-names:
+    items:
+      - const: hw
+
+  resets:
+    description:
+      physical line number to hardware reset the mmc
+    maxItems: 1
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
 
+allOf:
+  - $ref: mmc-controller.yaml
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: amd,pensando-elba-sd4hc
+    then:
+      required:
+        - reset-names
+        - resets
+      properties:
+        reg:
+          minItems: 2
+    else:
+      properties:
+        reset-names: false
+        resets: false
+        reg:
+          maxItems: 1
+

Regards,
Brad
Krzysztof Kozlowski Jan. 21, 2023, 6:57 p.m. UTC | #4 
On 21/01/2023 02:10, Brad Larson wrote:
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
>    - clocks
>  
> +allOf:
> +  - $ref: mmc-controller.yaml
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amd,pensando-elba-sd4hc
> +    then:
> +      required:
> +        - reset-names
> +        - resets

Looks correct, just put required: after properties: below.

> +      properties:
> +        reg:
> +          minItems: 2
> +    else:

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
index 8b1a0fdcb5e3..f7dd6f990f96 100644
--- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
@@ -16,12 +16,14 @@  properties:
   compatible:
     items:
       - enum:
+          - amd,pensando-elba-sd4hc
           - microchip,mpfs-sd4hc
           - socionext,uniphier-sd4hc
       - const: cdns,sd4hc
 
   reg:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   interrupts:
     maxItems: 1
@@ -111,12 +113,36 @@  properties:
     minimum: 0
     maximum: 0x7f
 
+  reset-names:
+    items:
+      - const: hw
+
+  resets:
+    description:
+      optional. phandle to the system reset controller with line index
+      for mmc hw reset line if exists.
+    maxItems: 1
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
 
+if:
+  properties:
+    compatible:
+      const: amd,pensando-elba-sd4hc
+then:
+  properties:
+    reg:
+      minItems: 2
+else:
+  properties:
+    reg:
+      minItems: 1
+      maxItems: 2
+
 unevaluatedProperties: false
 
 examples: