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

From: Brad Larson <blarson@amd.com>
To: <krzysztof.kozlowski@linaro.org>
Cc: <adrian.hunter@intel.com>, <alcooperx@gmail.com>,
	<andy.shevchenko@gmail.com>, <arnd@arndb.de>, <blarson@amd.com>,
	<brendan.higgins@linux.dev>, <briannorris@chromium.org>,
	<brijeshkumar.singh@amd.com>, <broonie@kernel.org>,
	<catalin.marinas@arm.com>, <davidgow@google.com>,
	<devicetree@vger.kernel.org>, <fancer.lancer@gmail.com>,
	<gerg@linux-m68k.org>, <gsomlo@gmail.com>, <krzk@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <lee.jones@linaro.org>,
	<lee@kernel.org>, <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <linux-mmc@vger.kernel.org>,
	<linux-spi@vger.kernel.org>, <p.yadav@ti.com>,
	<p.zabel@pengutronix.de>, <piotrs@cadence.com>,
	<rdunlap@infradead.org>, <robh+dt@kernel.org>,
	<samuel@sholland.org>, <skhan@linuxfoundation.org>,
	<suravee.suthikulpanit@amd.com>, <thomas.lendacky@amd.com>,
	<tonyhuang.sunplus@gmail.com>, <ulf.hansson@linaro.org>,
	<vaishnav.a@ti.com>, <will@kernel.org>,
	<yamada.masahiro@socionext.com>
Subject: Re: [PATCH v10 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC
Date: Mon, 6 Mar 2023 18:11:03 -0800	[thread overview]
Message-ID: <20230307021103.25299-1-blarson@amd.com> (raw)
In-Reply-To: <ebbc1c7a-2a96-409c-0e92-ae0f57ae5335@linaro.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel