Message-ID: <cded96da-fdb5-4a50-9382-8f9f19589ce8@gmail.com>
Date: Sat, 6 Sep 2025 14:13:17 +0530
From: Charan Pedumuru <charan.pedumuru@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: mmc: sdhci-omap: convert text based binding
 to json schema



On 28-05-2025 13:30, Krzysztof Kozlowski wrote:
> On 23/05/2025 19:05, Charan Pedumuru wrote:
>> Convert TI OMAP SDHCI Controller binding to YAML format.
>> Changes during Conversion:
>> - Add patternProperties for pinctrl-<n>.
>> - Define new properties like "ti,hwmods", "ti,needs-special-reset"
>>   "ti,needs-special-hs-handling", "cap-mmc-dual-data-rate"
>>   and "pbias-supply".
> 
> Why? commit should answer this.

The above properties are not documented in the text binding, so I defined them to resolve DTB_CHECK, I will write the reason in next revision.

> 
>> - Remove "ti,hwmods", "pinctrl-names" and "pinctrl-<n>"
> 
> Why? You just added ti,hwmods, so how can you remove it from required?

The property is defined but is not required by all DTS files and the old binding says it is required for all boards, I will add this reason to the commit message.

> 
>>   from required as they are not necessary for all DTS files.
>> - Add missing strings like "default-rev11", "sdr12-rev11", "sdr25-rev11",
>>   "hs-rev11", "sdr25-rev11" and "sleep" to pinctrl-names string array.
>>
>> Signed-off-by: Charan Pedumuru <charan.pedumuru@...il.com>
>> ---
>>  .../devicetree/bindings/mmc/sdhci-omap.txt         |  43 ------
>>  .../devicetree/bindings/mmc/sdhci-omap.yaml        | 155 +++++++++++++++++++++
> 
> 
> Filename: ti,omap-sdhci.yaml or one of the compatibles (or anything else
> following convention that it should match compatible).

Sure, I was following the name format of other files from the same directory here, but will change it to the compatible in next revision.

> 
> 
> "ti,needs-special-hs-handling" is already documented in other binding

Well, I didn't see this property defined in any common.yaml in mmc directory.

> 
> 
>>  2 files changed, 155 insertions(+), 43 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
>> deleted file mode 100644
>> index f91e341e6b36c410275e6f993dd08400be3fc1f8..0000000000000000000000000000000000000000
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
>> +++ /dev/null
>> @@ -1,43 +0,0 @@
>> -* TI OMAP SDHCI Controller
> 
> 
> ...
> 
> 
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.yaml b/Documentation/devicetree/bindings/mmc/sdhci-omap.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e707837bc242b055bbc497ed893a91c9b24f2dde
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.yaml
>> @@ -0,0 +1,155 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mmc/sdhci-omap.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TI OMAP SDHCI Controller
>> +
>> +maintainers:
>> +  - Ulf Hansson <ulf.hansson@...aro.org>
> 
> This is supposed to be someone caring about this device. Eventually
> platform maintainer.

Sure, I will change that, I was following the names of MAINTAINERS from the list I got from the command, "./scripts/get_maintainer.pl Documentation/dev
icetree/bindings/mmc/sdhci-omap.txt"

> 
>> +
>> +description:
>> +  For UHS devices which require tuning, the device tree should have a
>> +  cpu_thermal node which maps to the appropriate thermal zone. This
>> +  is used to get the temperature of the zone during tuning.
>> +
>> +allOf:
>> +  - $ref: sdhci-common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,omap2430-sdhci
>> +      - ti,omap3-sdhci
>> +      - ti,omap4-sdhci
>> +      - ti,omap5-sdhci
>> +      - ti,dra7-sdhci
>> +      - ti,k2g-sdhci
>> +      - ti,am335-sdhci
>> +      - ti,am437-sdhci
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  pinctrl-names:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    minItems: 1
>> +    maxItems: 19
>> +    items:
>> +      enum:
>> +        - default
>> +        - default-rev11
>> +        - hs
>> +        - sdr12
>> +        - sdr12-rev11
>> +        - sdr25
>> +        - sdr25-rev11
>> +        - sdr50
>> +        - ddr50-rev11
>> +        - sdr104-rev11
>> +        - ddr50
>> +        - sdr104
>> +        - ddr_1_8v-rev11
>> +        - ddr_1_8v
>> +        - ddr_3_3v
>> +        - hs-rev11
>> +        - hs200_1_8v-rev11
>> +        - hs200_1_8v
>> +        - sleep
>> +
>> +  dmas:
>> +    maxItems: 2
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx
>> +      - const: rx
>> +
>> +  ti,hwmods:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description:
>> +      This field is used to fetch the information such as
>> +      address range, irq lines, dma lines, interconnect, PRCM register,
>> +      clock domain, input clocks associated with MMC.
>> +    pattern: "^mmc[0-9]+$"
>> +
>> +  ti,needs-special-reset:
> 
> I don't understand why you added this. There is no user of it.


May be, but the DTB_CHECK failed for some boards when not defined it here.

> 
>> +    description:
>> +      It indicates that a specific soft reset sequence is required for
>> +      certain Texas Instruments devices, particularly those with
>> +      HSMMC (High-Speed MultiMediaCard) controllers.
>> +    type: boolean
>> +
>> +  ti,needs-special-hs-handling:
> 
> I don't understand why you added this. There is no user of it.

...

> 
> 
>> +    description:
>> +      It's presence in an MMC controller's DT node signals to the Linux kernel's
>> +      omap_hsmmc driver that this particular IP block requires special software
>> +      handling or workarounds to correctly manage High-Speed (HS) modes like
>> +      SDR25, SDR50, SDR104, DDR50.
>> +    type: boolean
>> +
>> +  pbias-supply:
>> +    description:
>> +      It is used to specify the voltage regulator that provides the bias
>> +      voltage for certain analog or I/O pads.
>> +
>> +  cap-mmc-dual-data-rate:
>> +    description:
>> +      A characteristic or capability associated with MultiMediaCard (MMC)
>> +      interfaces, specifically indicating that the MMC controller
>> +      supports Dual Data Rate (DDR) mode
> 
> Drop the property. We have standard properties for this and there is no
> ABI for it anyway.
> 

Same here, the DTB_CHECK failed, so had to define it here

>> +    type: boolean
>> +
>> +  ti,non-removable:
>> +    description:
>> +      It indicates that a component is not meant to be easily removed or
>> +      replaced by the user, such as an embedded battery or a non-removable
>> +      storage slot like eMMC.
>> +    type: boolean
>> +    deprecated: true
>> +
>> +  vmmmc-supply:
>> +    description:
>> +      It is used to specify the power supply (regulator) for the MMC/SD card's
>> +      main operating voltage (VCC/VDD).
>> +
>> +  clock-frequency:
> 
> Why is it here? Nothing in commit msg explained adding it.

I will add this change to commit message along with the reason.

> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      It is used to specify the frequency of a clock in Hertz (Hz). It's a
>> +      fundamental property for communicating hardware clocking information from
>> +      the Device Tree to the Linux kernel.
> 
> Redundant description. It is not a fundamental property. It is a legacy
> property.
> 

Sure, will change the description.

>> +
>> +patternProperties:
>> +  "^pinctrl-[0-9]+$":
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandles to pinctrl states. The numeric suffix determines the
>> +      state index corresponding to entries in the pinctrl-names array.
>> +    minItems: 1
> 
> Why exactly do you need these?

Some boards have this property with multiple pincontrol states, so had to define a pattern property to recognize all the defined pinctrl properties.

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +unevaluatedProperties: false
>> +
> Best regards,
> Krzysztof

-- 
Best Regards,
Charan.

Powered by blists - more mailing lists