[v1,1/3] dt-bindings: mmc: Add bindings for StarFive

  Add documentation to describe StarFive
designware mobile storage host controller driver.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
  

On Wed, 07 Dec 2022 21:17:29 +0800, William Qiu wrote:
> Add documentation to describe StarFive
> designware mobile storage host controller driver.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No such file or directory
   21 |         #include <dt-bindings/clock/starfive-jh7110.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221207131731.1291517-2-william.qiu@starfivetech.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.
  

On Wed, Dec 07, 2022 at 08:19:49AM -0600, Rob Herring wrote:
> 
> On Wed, 07 Dec 2022 21:17:29 +0800, William Qiu wrote:
> > Add documentation to describe StarFive
> > designware mobile storage host controller driver.
> > 
> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > ---
> >  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No such file or directory
>    21 |         #include <dt-bindings/clock/starfive-jh7110.h>
>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Hello William,
As with the other bindings that StarFive has sent out recently,
including this header in the example creates a dependency on the
clock/reset stuff being merged prior to the drivers for unrelated
subsytems.
To avoid that, you can drop the headers & definitions from these
*examples* & use the numbers themselves instead.

That way, the only thing that depends on the clock binding headers is
the DeviceTree patch & the driver can be merged once it is ready.

Thanks,
Conor.

> compilation terminated.
> make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1492: dt_binding_check] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221207131731.1291517-2-william.qiu@starfivetech.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.
>
  

On 07/12/2022 14:17, William Qiu wrote:
> Add documentation to describe StarFive
> designware mobile storage host controller driver.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> new file mode 100644
> index 000000000000..4f27ef3cf4f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/starfive,jh7110-sdio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive Designware Mobile Storage Host Controller
> +
> +description:
> +  StarFive uses the Synopsys designware mobile storage host controller
> +  to interface a SoC with storage medium such as eMMC or SD/MMC cards.
> +
> +allOf:
> +  - $ref: "synopsys-dw-mshc-common.yaml#"

Drop quotes

> +
> +maintainers:
> +  - William Qiu <william.qiu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-sdio

Why do you call it sdio if the interface is for mmc as well?

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    items:
> +      - description: biu clock
> +      - description: ciu clock

I don't think the card interface clock is optional... are you sure you
have designs working without it? No clock line at all getting to the memory?

> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: biu
> +      - const: ciu
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  starfive,sys-syscon:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      The desired number of times that the host execute tuning when needed.

That's not matching the property name. Missing number of items... this
is anyway confusing. Why number of tuning tries is a property of DT?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/starfive-jh7110.h>
> +    #include <dt-bindings/reset/starfive-jh7110.h>
> +    mmc@16010000 {
> +            compatible = "starfive,jh7110-sdio";

Use 4 spaces for example indentation.

> +            reg = <0x16010000 0x10000>;
> +            clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
> +                 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;

Align with previous <

Best regards,
Krzysztof
  

On 2022/12/7 22:19, Rob Herring wrote:
> 
> On Wed, 07 Dec 2022 21:17:29 +0800, William Qiu wrote:
>> Add documentation to describe StarFive
>> designware mobile storage host controller driver.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
>>  1 file changed, 71 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No such file or directory
>    21 |         #include <dt-bindings/clock/starfive-jh7110.h>
>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1492: dt_binding_check] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221207131731.1291517-2-william.qiu@starfivetech.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.
> 

Hi Rob Herring,

Thank you for taking time to review and provide helpful comments for this patch.
I'll take Conor's suggestion and replace the IDs and drop the header then.

Best regards,
William Qiu
  

On 2022/12/7 22:46, Conor Dooley wrote:
> On Wed, Dec 07, 2022 at 08:19:49AM -0600, Rob Herring wrote:
>> 
>> On Wed, 07 Dec 2022 21:17:29 +0800, William Qiu wrote:
>> > Add documentation to describe StarFive
>> > designware mobile storage host controller driver.
>> > 
>> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> > ---
>> >  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
>> >  1 file changed, 71 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> > 
>> 
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>> 
>> yamllint warnings/errors:
>> 
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No such file or directory
>>    21 |         #include <dt-bindings/clock/starfive-jh7110.h>
>>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Hello William,
> As with the other bindings that StarFive has sent out recently,
> including this header in the example creates a dependency on the
> clock/reset stuff being merged prior to the drivers for unrelated
> subsytems.
> To avoid that, you can drop the headers & definitions from these
> *examples* & use the numbers themselves instead.
> 
> That way, the only thing that depends on the clock binding headers is
> the DeviceTree patch & the driver can be merged once it is ready.
> 
> Thanks,
> Conor.
> 
Hi Conor,

Thank you for your suggestion. I will take it and fix it.

Best regards,
William Qiu

>> compilation terminated.
>> make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.example.dtb] Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> make: *** [Makefile:1492: dt_binding_check] Error 2
>> 
>> doc reference errors (make refcheckdocs):
>> 
>> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221207131731.1291517-2-william.qiu@starfivetech.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.
>>
  

On 2022/12/7 23:13, Krzysztof Kozlowski wrote:
> On 07/12/2022 14:17, William Qiu wrote:
>> Add documentation to describe StarFive
>> designware mobile storage host controller driver.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
>>  1 file changed, 71 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> new file mode 100644
>> index 000000000000..4f27ef3cf4f3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> @@ -0,0 +1,71 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mmc/starfive,jh7110-sdio.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive Designware Mobile Storage Host Controller
>> +
>> +description:
>> +  StarFive uses the Synopsys designware mobile storage host controller
>> +  to interface a SoC with storage medium such as eMMC or SD/MMC cards.
>> +
>> +allOf:
>> +  - $ref: "synopsys-dw-mshc-common.yaml#"
> 
> Drop quotes
> 

Will fix.

>> +
>> +maintainers:
>> +  - William Qiu <william.qiu@starfivetech.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: starfive,jh7110-sdio
> 
> Why do you call it sdio if the interface is for mmc as well?
> 

Will update compatible.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 1
>> +    items:
>> +      - description: biu clock
>> +      - description: ciu clock
> 
> I don't think the card interface clock is optional... are you sure you
> have designs working without it? No clock line at all getting to the memory?
> 

Will fix.

>> +
>> +  clock-names:
>> +    minItems: 1
>> +    items:
>> +      - const: biu
>> +      - const: ciu
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  starfive,sys-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    description:
>> +      The desired number of times that the host execute tuning when needed.
> 
> That's not matching the property name. Missing number of items... this
> is anyway confusing. Why number of tuning tries is a property of DT?
> 

Will update the description.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/starfive-jh7110.h>
>> +    #include <dt-bindings/reset/starfive-jh7110.h>
>> +    mmc@16010000 {
>> +            compatible = "starfive,jh7110-sdio";
> 
> Use 4 spaces for example indentation.
> 

I'll fix the indentation.

>> +            reg = <0x16010000 0x10000>;
>> +            clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
>> +                 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
> 
> Align with previous <
> 

I will fix.

Thank you for taking time to review and provide helpful comments for this patch.

Best regards,
William Qiu

> Best regards,
> Krzysztof
>
  

On 08/12/2022 09:44, William Qiu wrote:
> 
>>> +
>>> +  clock-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: biu
>>> +      - const: ciu
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  starfive,sys-syscon:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    description:
>>> +      The desired number of times that the host execute tuning when needed.
>>
>> That's not matching the property name. Missing number of items... this
>> is anyway confusing. Why number of tuning tries is a property of DT?
>>
> 
> Will update the description.

I propose first to explain what is it. Because it is not about
description only, but also type. Your driver uses this as syscon, so
this cannot be uint32-array but phandle-array...

Best regards,
Krzysztof
  

On 2022/12/8 17:01, Krzysztof Kozlowski wrote:
> On 08/12/2022 09:44, William Qiu wrote:
>> 
>>>> +
>>>> +  clock-names:
>>>> +    minItems: 1
>>>> +    items:
>>>> +      - const: biu
>>>> +      - const: ciu
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  starfive,sys-syscon:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +    description:
>>>> +      The desired number of times that the host execute tuning when needed.
>>>
>>> That's not matching the property name. Missing number of items... this
>>> is anyway confusing. Why number of tuning tries is a property of DT?
>>>
>> 
>> Will update the description.
> 
> I propose first to explain what is it. Because it is not about
> description only, but also type. Your driver uses this as syscon, so
> this cannot be uint32-array but phandle-array...
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof,

Thank you for suggestion and I'll take it. I'll update it in next
version.

Best regards,
William Qiu
  

Hi William,

thanks for your patch!

On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote:

> Add documentation to describe StarFive
> designware mobile storage host controller driver.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>

(...)

> +  starfive,sys-syscon:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      The desired number of times that the host execute tuning when needed.

This is not consistent with the use in the code of the attached driver.
There it is a phandle, and it has three cells, which I am critical of.
Also this description is hard to understand.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts

I don't think the syscon phandle is optional.

> +    #include <dt-bindings/clock/starfive-jh7110.h>
> +    #include <dt-bindings/reset/starfive-jh7110.h>
> +    mmc@16010000 {
> +            compatible = "starfive,jh7110-sdio";
> +            reg = <0x16010000 0x10000>;

No syscon phandle in the example: this needs to be added.

Yours,
Linus Walleij
  

On 2022/12/9 5:13, Linus Walleij wrote:
> Hi William,
> 
> thanks for your patch!
> 
> On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote:
> 
>> Add documentation to describe StarFive
>> designware mobile storage host controller driver.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> 
> (...)
> 
>> +  starfive,sys-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    description:
>> +      The desired number of times that the host execute tuning when needed.
> 
> This is not consistent with the use in the code of the attached driver.
> There it is a phandle, and it has three cells, which I am critical of.
> Also this description is hard to understand.
> 

Will update all of it in next version.


>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
> 
> I don't think the syscon phandle is optional.
> 

Will fix.

>> +    #include <dt-bindings/clock/starfive-jh7110.h>
>> +    #include <dt-bindings/reset/starfive-jh7110.h>
>> +    mmc@16010000 {
>> +            compatible = "starfive,jh7110-sdio";
>> +            reg = <0x16010000 0x10000>;
> 
> No syscon phandle in the example: this needs to be added.
> 

I will add the syscon phandle.

Thank you for taking time to review and provide helpful comments for this patch.

Best regards,
William Qiu
> Yours,
> Linus Walleij