Re: [PATCH v10 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC

[Brad Larson <blarson@amd.com>]

On 06/03/2023 8:28, Krzysztof Kozlowski wrote: 
> On 06/03/2023 05:07, 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>
>> ---
>> 
>> v10 changes:
>> - Move reset-names property definition next to existing resets prop
>> - Move allOf to the bottom and set resets/reset-names required only for pensando
>> - Fix reg maxItems for existing, must be 1
>> 
>> v9 changes:
>> - 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   | 33 ++++++++++++++++---
>>  1 file changed, 29 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
>> index adacd0535c14..0c4d6d4b2b58 100644
>> --- 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
>> @@ -30,8 +29,13 @@ properties:
>>      maxItems: 1
>>  
>>    resets:
>> +    description: physical line number to hardware reset the mmc
>
> This part seems to be not needed anymore. Resets field was already added.

Yes, see below.

>
>>      maxItems: 1
>>  
>> +  reset-names:
>> +    items:
>> +      - const: hw
>
> Why did you add reset-names for one item? There is no v8 of this patch,
> so I cannot find previous discussion about it.

I found resets property was added recently when I rebased.

cb7f090171393 (Kunihiko Hayashi    2023-02-13 13:52:33 +0900  32)   resets:
cb7f090171393 (Kunihiko Hayashi    2023-02-13 13:52:33 +0900  33)     maxItems: 1

I've deleted reset-names and dropped description for resets.

>
>>    # PHY DLL input delays:
>>    # They are used to delay the data valid window, and align the window to
>>    # sampling clock. The delay starts from 5ns (for delay parameter equal to 0)
>> @@ -120,6 +124,27 @@ required:
>>    - interrupts
>>    - clocks
>>  
>> +allOf:
>> +  - $ref: mmc-controller.yaml
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: amd,pensando-elba-sd4hc
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 2
>
> Hm, we missed to mention it before, but what is the second reg for? It's
> not obvious from the binding so probably you need to describe it instead
> minItems:
>   items:
>     - description: foo
>     - description: bar
>

The second reg is byte lane enable for writes.  The following passed dtbs_check
after getting: hint: "minItems" is only needed if less than the "items" list length

    then:
      properties:
        reg:
          items:
            - description: Host controller registers
            - description: Elba byte-lane enable register for writes
      required:
        - resets
    else:
      properties:
        resets: false
        reg:
          maxItems: 1

>> +      required:
>> +        - reset-names
>> +        - resets
>> +    else:
>> +      properties:
>> +        reset-names: false
>> +        resets: false
>> +        reg:
>> +          maxItems: 1
>> +
>>  unevaluatedProperties: false

Regards,
Brad