[PATCH v1 0/3] StarFive's SDIO/eMMC driver support

[PATCH v1 0/3] StarFive's SDIO/eMMC driver support

[William Qiu]

Hi,

This patchset adds initial rudimentary support for the StarFive
designware mobile storage host controller driver. And this driver will
be used in StarFive's visionfive-v2 board. The main purpose of adding
this driver is to accommodate the ultra-high speed mode of eMMC.

The patch series is based on v6.1-rc5.

-- William

William Qiu (3):
  dt-bindings: mmc: Add bindings for StarFive
  mmc: starfive: Add sdio/emmc driver support
  riscv: dts: starfive: Add mmc node

 .../bindings/mmc/starfive,jh7110-sdio.yaml    |  71 +++++++
 MAINTAINERS                                   |   6 +
 .../jh7110-starfive-visionfive-v2.dts         |  25 +++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  38 ++++
 drivers/mmc/host/Kconfig                      |  10 +
 drivers/mmc/host/Makefile                     |   1 +
 drivers/mmc/host/dw_mmc-starfive.c            | 197 ++++++++++++++++++
 7 files changed, 348 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
 create mode 100644 drivers/mmc/host/dw_mmc-starfive.c

--
2.34.1

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

[William Qiu]

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#"
+
+maintainers:
+  - William Qiu <william.qiu@starfivetech.com>
+
+properties:
+  compatible:
+    const: starfive,jh7110-sdio
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    items:
+      - description: biu clock
+      - description: ciu clock
+
+  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.
+
+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";
+            reg = <0x16010000 0x10000>;
+            clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
+                 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
+            clock-names = "biu","ciu";
+            resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
+            reset-names = "reset";
+            interrupts = <74>;
+            fifo-depth = <32>;
+            fifo-watermark-aligned;
+            data-addr = <0>;
+    };
-- 
2.34.1

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

[Rob Herring]

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.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

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.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[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.
>> 

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

[William Qiu]

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

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

[Krzysztof Kozlowski]

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

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

[William Qiu]

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
> 

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

[Krzysztof Kozlowski]

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

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

[William Qiu]

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

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

[Linus Walleij]

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

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

[William Qiu]

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

[PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[William Qiu]

Add sdio/emmc driver support for StarFive JH7110 soc.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 MAINTAINERS                        |   6 +
 drivers/mmc/host/Kconfig           |  10 ++
 drivers/mmc/host/Makefile          |   1 +
 drivers/mmc/host/dw_mmc-starfive.c | 197 +++++++++++++++++++++++++++++
 4 files changed, 214 insertions(+)
 create mode 100644 drivers/mmc/host/dw_mmc-starfive.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a70c1d0f303e..2b46ef07f5dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19623,6 +19623,12 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
 F:	drivers/reset/starfive/
 F:	include/dt-bindings/reset/starfive*
 
+STARFIVE JH7110 MMC/SD/SDIO DRIVER
+M:	William Qiu <william.qiu@starfivetech.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/mmc/starfive*
+F:	drivers/mmc/dw_mmc-starfive.c
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <peterz@infradead.org>
 M:	Josh Poimboeuf <jpoimboe@kernel.org>
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index fb1062a6394c..b87262503403 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -871,6 +871,16 @@ config MMC_DW_ROCKCHIP
 	  Synopsys DesignWare Memory Card Interface driver. Select this option
 	  for platforms based on RK3066, RK3188 and RK3288 SoC's.
 
+config MMC_DW_STARFIVE
+	tristate "StarFive specific extensions for Synopsys DW Memory Card Interface"
+	depends on SOC_STARFIVE
+	depends on MMC_DW
+	select MMC_DW_PLTFM
+	help
+	  This selects support for StarFive JH7110 SoC specific extensions to the
+	  Synopsys DesignWare Memory Card Interface driver. Select this option
+	  for platforms based on StarFive JH7110 SoC.
+
 config MMC_SH_MMCIF
 	tristate "SuperH Internal MMCIF support"
 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 4e4ceb32c4b4..32c0e5564b9a 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_MMC_DW_HI3798CV200) += dw_mmc-hi3798cv200.o
 obj-$(CONFIG_MMC_DW_K3)		+= dw_mmc-k3.o
 obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
 obj-$(CONFIG_MMC_DW_ROCKCHIP)	+= dw_mmc-rockchip.o
+obj-$(CONFIG_MMC_DW_STARFIVE)	+= dw_mmc-starfive.o
 obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
 obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o
 obj-$(CONFIG_MMC_VUB300)	+= vub300.o
diff --git a/drivers/mmc/host/dw_mmc-starfive.c b/drivers/mmc/host/dw_mmc-starfive.c
new file mode 100644
index 000000000000..b2e6a0b6abf9
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-starfive.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StarFive Designware Mobile Storage Host Controller Driver
+ *
+ * Copyright (c) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mmc/host.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+#define ALL_INT_CLR		0x1ffff
+#define MAX_DELAY_CHAIN		32
+
+struct starfive_priv {
+	struct device *dev;
+	struct regmap *reg_syscon;
+	u32 syscon_offset;
+	u32 syscon_shift;
+	u32 syscon_mask;
+};
+
+static unsigned long dw_mci_starfive_caps[] = {
+	MMC_CAP_CMD23,
+	MMC_CAP_CMD23,
+	MMC_CAP_CMD23
+};
+
+static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	int ret;
+	unsigned int clock;
+
+	if (ios->timing == MMC_TIMING_MMC_DDR52 || ios->timing == MMC_TIMING_UHS_DDR50) {
+		clock = (ios->clock > 50000000 && ios->clock <= 52000000) ? 100000000 : ios->clock;
+		ret = clk_set_rate(host->ciu_clk, clock);
+		if (ret)
+			dev_dbg(host->dev, "Use an external frequency divider %uHz\n", ios->clock);
+		host->bus_hz = clk_get_rate(host->ciu_clk);
+	} else {
+		dev_dbg(host->dev, "Using the internal divider\n");
+	}
+}
+
+static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
+					     u32 opcode)
+{
+	static const int grade  = MAX_DELAY_CHAIN;
+	struct dw_mci *host = slot->host;
+	struct starfive_priv *priv = host->priv;
+	int raise_point = -1, fall_point = -1;
+	int err, prev_err = -1;
+	int found = 0;
+	int i;
+	u32 regval;
+
+	for (i = 0; i < grade; i++) {
+		regval = i << priv->syscon_shift;
+		err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
+						priv->syscon_mask, regval);
+		if (err)
+			return err;
+		mci_writel(host, RINTSTS, ALL_INT_CLR);
+
+		err = mmc_send_tuning(slot->mmc, opcode, NULL);
+		if (!err)
+			found = 1;
+
+		if (i > 0) {
+			if (err && !prev_err)
+				fall_point = i - 1;
+			if (!err && prev_err)
+				raise_point = i;
+		}
+
+		if (raise_point != -1 && fall_point != -1)
+			goto tuning_out;
+
+		prev_err = err;
+		err = 0;
+	}
+
+tuning_out:
+	if (found) {
+		if (raise_point == -1)
+			raise_point = 0;
+		if (fall_point == -1)
+			fall_point = grade - 1;
+		if (fall_point < raise_point) {
+			if ((raise_point + fall_point) >
+			    (grade - 1))
+				i = fall_point / 2;
+			else
+				i = (raise_point + grade - 1) / 2;
+		} else {
+			i = (raise_point + fall_point) / 2;
+		}
+
+		regval = i << priv->syscon_shift;
+		err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
+						priv->syscon_mask, regval);
+		if (err)
+			return err;
+		mci_writel(host, RINTSTS, ALL_INT_CLR);
+
+		dev_dbg(host->dev, "Found valid delay chain! use it [delay=%d]\n", i);
+	} else {
+		dev_err(host->dev, "No valid delay chain! use default\n");
+		err = -EINVAL;
+	}
+
+	mci_writel(host, RINTSTS, ALL_INT_CLR);
+	return err;
+}
+
+static int dw_mci_starfive_parse_dt(struct dw_mci *host)
+{
+	struct of_phandle_args args;
+	struct starfive_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
+						"starfive,sys-syscon", 3, 0, &args);
+	if (ret) {
+		dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
+		return -EINVAL;
+	}
+
+	priv->reg_syscon = syscon_node_to_regmap(args.np);
+	of_node_put(args.np);
+	if (IS_ERR(priv->reg_syscon))
+		return PTR_ERR(priv->reg_syscon);
+
+	priv->syscon_offset = args.args[0];
+	priv->syscon_shift  = args.args[1];
+	priv->syscon_mask   = args.args[2];
+
+	host->priv = priv;
+
+	return 0;
+}
+
+static const struct dw_mci_drv_data starfive_data = {
+	.caps = dw_mci_starfive_caps,
+	.num_caps = ARRAY_SIZE(dw_mci_starfive_caps),
+	.set_ios = dw_mci_starfive_set_ios,
+	.parse_dt = dw_mci_starfive_parse_dt,
+	.execute_tuning = dw_mci_starfive_execute_tuning,
+};
+
+static const struct of_device_id dw_mci_starfive_match[] = {
+	{ .compatible = "starfive,jh7110-sdio",
+		.data = &starfive_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_starfive_match);
+
+static int dw_mci_starfive_probe(struct platform_device *pdev)
+{
+	return dw_mci_pltfm_register(pdev, &starfive_data);
+}
+
+static int dw_mci_starfive_remove(struct platform_device *pdev)
+{
+	return dw_mci_pltfm_remove(pdev);
+}
+
+static struct platform_driver dw_mci_starfive_driver = {
+	.probe = dw_mci_starfive_probe,
+	.remove = dw_mci_starfive_remove,
+	.driver = {
+		.name = "dwmmc_starfive",
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = dw_mci_starfive_match,
+	},
+};
+module_platform_driver(dw_mci_starfive_driver);
+
+MODULE_DESCRIPTION("StarFive JH7110 Specific DW-MSHC Driver Extension");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:dwmmc_starfive");
-- 
2.34.1

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[Linus Walleij]

Hi William,

thanks for your patch!

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

> Add sdio/emmc driver support for StarFive JH7110 soc.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>

(...)
> +#include <linux/gpio.h>

Never include this legacy header in new code. Also: you don't use it.

> +#include <linux/mfd/syscon.h>
> +#include <linux/mmc/host.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

You're not using this include either.

> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

Or this.

> +#define ALL_INT_CLR            0x1ffff
> +#define MAX_DELAY_CHAIN                32
> +
> +struct starfive_priv {
> +       struct device *dev;
> +       struct regmap *reg_syscon;
> +       u32 syscon_offset;
> +       u32 syscon_shift;
> +       u32 syscon_mask;
> +};
> +
> +static unsigned long dw_mci_starfive_caps[] = {
> +       MMC_CAP_CMD23,
> +       MMC_CAP_CMD23,
> +       MMC_CAP_CMD23
> +};
> +
> +static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> +{
> +       int ret;
> +       unsigned int clock;
> +
> +       if (ios->timing == MMC_TIMING_MMC_DDR52 || ios->timing == MMC_TIMING_UHS_DDR50) {
> +               clock = (ios->clock > 50000000 && ios->clock <= 52000000) ? 100000000 : ios->clock;
> +               ret = clk_set_rate(host->ciu_clk, clock);
> +               if (ret)
> +                       dev_dbg(host->dev, "Use an external frequency divider %uHz\n", ios->clock);
> +               host->bus_hz = clk_get_rate(host->ciu_clk);
> +       } else {
> +               dev_dbg(host->dev, "Using the internal divider\n");
> +       }
> +}
> +
> +static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
> +                                            u32 opcode)
> +{
> +       static const int grade  = MAX_DELAY_CHAIN;
> +       struct dw_mci *host = slot->host;
> +       struct starfive_priv *priv = host->priv;
> +       int raise_point = -1, fall_point = -1;
> +       int err, prev_err = -1;

I don't like these default-init to -1. Can you just skip it and assign it
where it makes most sense instead?

> +       int found = 0;

This looks like a bool.

> +       int i;
> +       u32 regval;
> +
> +       for (i = 0; i < grade; i++) {
> +               regval = i << priv->syscon_shift;
> +               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
> +                                               priv->syscon_mask, regval);
> +               if (err)
> +                       return err;
> +               mci_writel(host, RINTSTS, ALL_INT_CLR);
> +
> +               err = mmc_send_tuning(slot->mmc, opcode, NULL);
> +               if (!err)
> +                       found = 1;
> +
> +               if (i > 0) {
> +                       if (err && !prev_err)
> +                               fall_point = i - 1;
> +                       if (!err && prev_err)
> +                               raise_point = i;
> +               }
> +
> +               if (raise_point != -1 && fall_point != -1)
> +                       goto tuning_out;

There are just these raise point (shouldn't this be "rise_point" in proper
english?) and fall point, this misses some comments explaining what is
going on, the code is not intuitively eviden. Rise and fall of *what* for
example.

> +
> +               prev_err = err;
> +               err = 0;
> +       }
> +
> +tuning_out:
> +       if (found) {
> +               if (raise_point == -1)
> +                       raise_point = 0;
> +               if (fall_point == -1)
> +                       fall_point = grade - 1;
> +               if (fall_point < raise_point) {
> +                       if ((raise_point + fall_point) >
> +                           (grade - 1))
> +                               i = fall_point / 2;
> +                       else
> +                               i = (raise_point + grade - 1) / 2;
> +               } else {
> +                       i = (raise_point + fall_point) / 2;
> +               }

Likewise here, explain what grade is, refer to the eMMC spec if necessary.

(...)
> +       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
> +                                               "starfive,sys-syscon", 3, 0, &args);
> +       if (ret) {
> +               dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
> +               return -EINVAL;
> +       }
> +
> +       priv->reg_syscon = syscon_node_to_regmap(args.np);
> +       of_node_put(args.np);
> +       if (IS_ERR(priv->reg_syscon))
> +               return PTR_ERR(priv->reg_syscon);
> +
> +       priv->syscon_offset = args.args[0];
> +       priv->syscon_shift  = args.args[1];
> +       priv->syscon_mask   = args.args[2];

Why should these three things be in the device tree instead of being derived
from the compatible-string or just plain hard-coded as #defines?
I don't get it.

> +static int dw_mci_starfive_probe(struct platform_device *pdev)
> +{
> +       return dw_mci_pltfm_register(pdev, &starfive_data);
> +}
> +
> +static int dw_mci_starfive_remove(struct platform_device *pdev)
> +{
> +       return dw_mci_pltfm_remove(pdev);
> +}

Can't you just assign dw_mci_pltfm_remove() to .remove?

Other than these things, the driver looks good!

Yours,
Linus Walleij

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[William Qiu]

On 2022/12/9 5:09, 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 sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> 
> (...)
>> +#include <linux/gpio.h>
> 
> Never include this legacy header in new code. Also: you don't use it.
> 

Will fix.

>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
> 
> You're not using this include either.
> 

Will fix.

>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
> 
> Or this.
> 

Will fix.

>> +#define ALL_INT_CLR            0x1ffff
>> +#define MAX_DELAY_CHAIN                32
>> +
>> +struct starfive_priv {
>> +       struct device *dev;
>> +       struct regmap *reg_syscon;
>> +       u32 syscon_offset;
>> +       u32 syscon_shift;
>> +       u32 syscon_mask;
>> +};
>> +
>> +static unsigned long dw_mci_starfive_caps[] = {
>> +       MMC_CAP_CMD23,
>> +       MMC_CAP_CMD23,
>> +       MMC_CAP_CMD23
>> +};
>> +
>> +static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>> +{
>> +       int ret;
>> +       unsigned int clock;
>> +
>> +       if (ios->timing == MMC_TIMING_MMC_DDR52 || ios->timing == MMC_TIMING_UHS_DDR50) {
>> +               clock = (ios->clock > 50000000 && ios->clock <= 52000000) ? 100000000 : ios->clock;
>> +               ret = clk_set_rate(host->ciu_clk, clock);
>> +               if (ret)
>> +                       dev_dbg(host->dev, "Use an external frequency divider %uHz\n", ios->clock);
>> +               host->bus_hz = clk_get_rate(host->ciu_clk);
>> +       } else {
>> +               dev_dbg(host->dev, "Using the internal divider\n");
>> +       }
>> +}
>> +
>> +static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
>> +                                            u32 opcode)
>> +{
>> +       static const int grade  = MAX_DELAY_CHAIN;
>> +       struct dw_mci *host = slot->host;
>> +       struct starfive_priv *priv = host->priv;
>> +       int raise_point = -1, fall_point = -1;
>> +       int err, prev_err = -1;
> 
> I don't like these default-init to -1. Can you just skip it and assign it
> where it makes most sense instead?
> 

Will fix.

>> +       int found = 0;
> 
> This looks like a bool.
> 

Will update.

>> +       int i;
>> +       u32 regval;
>> +
>> +       for (i = 0; i < grade; i++) {
>> +               regval = i << priv->syscon_shift;
>> +               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
>> +                                               priv->syscon_mask, regval);
>> +               if (err)
>> +                       return err;
>> +               mci_writel(host, RINTSTS, ALL_INT_CLR);
>> +
>> +               err = mmc_send_tuning(slot->mmc, opcode, NULL);
>> +               if (!err)
>> +                       found = 1;
>> +
>> +               if (i > 0) {
>> +                       if (err && !prev_err)
>> +                               fall_point = i - 1;
>> +                       if (!err && prev_err)
>> +                               raise_point = i;
>> +               }
>> +
>> +               if (raise_point != -1 && fall_point != -1)
>> +                       goto tuning_out;
> 
> There are just these raise point (shouldn't this be "rise_point" in proper
> english?) and fall point, this misses some comments explaining what is
> going on, the code is not intuitively eviden. Rise and fall of *what* for
> example.
> 

I'll update it in next version.

>> +
>> +               prev_err = err;
>> +               err = 0;
>> +       }
>> +
>> +tuning_out:
>> +       if (found) {
>> +               if (raise_point == -1)
>> +                       raise_point = 0;
>> +               if (fall_point == -1)
>> +                       fall_point = grade - 1;
>> +               if (fall_point < raise_point) {
>> +                       if ((raise_point + fall_point) >
>> +                           (grade - 1))
>> +                               i = fall_point / 2;
>> +                       else
>> +                               i = (raise_point + grade - 1) / 2;
>> +               } else {
>> +                       i = (raise_point + fall_point) / 2;
>> +               }
> 
> Likewise here, explain what grade is, refer to the eMMC spec if necessary.
> 

Will update.

> (...)
>> +       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
>> +                                               "starfive,sys-syscon", 3, 0, &args);
>> +       if (ret) {
>> +               dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       priv->reg_syscon = syscon_node_to_regmap(args.np);
>> +       of_node_put(args.np);
>> +       if (IS_ERR(priv->reg_syscon))
>> +               return PTR_ERR(priv->reg_syscon);
>> +
>> +       priv->syscon_offset = args.args[0];
>> +       priv->syscon_shift  = args.args[1];
>> +       priv->syscon_mask   = args.args[2];
> 
> Why should these three things be in the device tree instead of being derived
> from the compatible-string or just plain hard-coded as #defines?
> I don't get it.
> 

Will update.

>> +static int dw_mci_starfive_probe(struct platform_device *pdev)
>> +{
>> +       return dw_mci_pltfm_register(pdev, &starfive_data);
>> +}
>> +
>> +static int dw_mci_starfive_remove(struct platform_device *pdev)
>> +{
>> +       return dw_mci_pltfm_remove(pdev);
>> +}
> 
> Can't you just assign dw_mci_pltfm_remove() to .remove?
> 

Will fix.

> Other than these things, the driver looks good!
> 

Hi Linus,

Thank you for taking time to review and provide helpful comments for this patch.
I will take all of your suggestions and update this driver in next version.

Best Regards
William Qiu
> Yours,
> Linus Walleij

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[William Qiu]

On 2022/12/9 5:09, 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 sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> 
> (...)
>> +#include <linux/gpio.h>
> 
> Never include this legacy header in new code. Also: you don't use it.
> 
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
> 
> You're not using this include either.
> 
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
> 
> Or this.
> 
>> +#define ALL_INT_CLR            0x1ffff
>> +#define MAX_DELAY_CHAIN                32
>> +
>> +struct starfive_priv {
>> +       struct device *dev;
>> +       struct regmap *reg_syscon;
>> +       u32 syscon_offset;
>> +       u32 syscon_shift;
>> +       u32 syscon_mask;
>> +};
>> +
>> +static unsigned long dw_mci_starfive_caps[] = {
>> +       MMC_CAP_CMD23,
>> +       MMC_CAP_CMD23,
>> +       MMC_CAP_CMD23
>> +};
>> +
>> +static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>> +{
>> +       int ret;
>> +       unsigned int clock;
>> +
>> +       if (ios->timing == MMC_TIMING_MMC_DDR52 || ios->timing == MMC_TIMING_UHS_DDR50) {
>> +               clock = (ios->clock > 50000000 && ios->clock <= 52000000) ? 100000000 : ios->clock;
>> +               ret = clk_set_rate(host->ciu_clk, clock);
>> +               if (ret)
>> +                       dev_dbg(host->dev, "Use an external frequency divider %uHz\n", ios->clock);
>> +               host->bus_hz = clk_get_rate(host->ciu_clk);
>> +       } else {
>> +               dev_dbg(host->dev, "Using the internal divider\n");
>> +       }
>> +}
>> +
>> +static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
>> +                                            u32 opcode)
>> +{
>> +       static const int grade  = MAX_DELAY_CHAIN;
>> +       struct dw_mci *host = slot->host;
>> +       struct starfive_priv *priv = host->priv;
>> +       int raise_point = -1, fall_point = -1;
>> +       int err, prev_err = -1;
> 
> I don't like these default-init to -1. Can you just skip it and assign it
> where it makes most sense instead?
> 
>> +       int found = 0;
> 
> This looks like a bool.
> 
>> +       int i;
>> +       u32 regval;
>> +
>> +       for (i = 0; i < grade; i++) {
>> +               regval = i << priv->syscon_shift;
>> +               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
>> +                                               priv->syscon_mask, regval);
>> +               if (err)
>> +                       return err;
>> +               mci_writel(host, RINTSTS, ALL_INT_CLR);
>> +
>> +               err = mmc_send_tuning(slot->mmc, opcode, NULL);
>> +               if (!err)
>> +                       found = 1;
>> +
>> +               if (i > 0) {
>> +                       if (err && !prev_err)
>> +                               fall_point = i - 1;
>> +                       if (!err && prev_err)
>> +                               raise_point = i;
>> +               }
>> +
>> +               if (raise_point != -1 && fall_point != -1)
>> +                       goto tuning_out;
> 
> There are just these raise point (shouldn't this be "rise_point" in proper
> english?) and fall point, this misses some comments explaining what is
> going on, the code is not intuitively eviden. Rise and fall of *what* for
> example.
> 
>> +
>> +               prev_err = err;
>> +               err = 0;
>> +       }
>> +
>> +tuning_out:
>> +       if (found) {
>> +               if (raise_point == -1)
>> +                       raise_point = 0;
>> +               if (fall_point == -1)
>> +                       fall_point = grade - 1;
>> +               if (fall_point < raise_point) {
>> +                       if ((raise_point + fall_point) >
>> +                           (grade - 1))
>> +                               i = fall_point / 2;
>> +                       else
>> +                               i = (raise_point + grade - 1) / 2;
>> +               } else {
>> +                       i = (raise_point + fall_point) / 2;
>> +               }
> 
> Likewise here, explain what grade is, refer to the eMMC spec if necessary.
> 
> (...)
>> +       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
>> +                                               "starfive,sys-syscon", 3, 0, &args);
>> +       if (ret) {
>> +               dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       priv->reg_syscon = syscon_node_to_regmap(args.np);
>> +       of_node_put(args.np);
>> +       if (IS_ERR(priv->reg_syscon))
>> +               return PTR_ERR(priv->reg_syscon);
>> +
>> +       priv->syscon_offset = args.args[0];
>> +       priv->syscon_shift  = args.args[1];
>> +       priv->syscon_mask   = args.args[2];
> 
> Why should these three things be in the device tree instead of being derived
> from the compatible-string or just plain hard-coded as #defines?
> I don't get it.
> 
Hi Linus,

I'm sorry to bother you, but as for the definition of syscon, after discussing with 
my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in
the device tree, and the code compatibility is better.

Best Regards
William Qiu
>> +static int dw_mci_starfive_probe(struct platform_device *pdev)
>> +{
>> +       return dw_mci_pltfm_register(pdev, &starfive_data);
>> +}
>> +
>> +static int dw_mci_starfive_remove(struct platform_device *pdev)
>> +{
>> +       return dw_mci_pltfm_remove(pdev);
>> +}
> 
> Can't you just assign dw_mci_pltfm_remove() to .remove?
> 
> Other than these things, the driver looks good!
> 
> Yours,
> Linus Walleij

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[Linus Walleij]

On Thu, Feb 2, 2023 at 12:10 PM William Qiu
<william.qiu@starfivetech.com> wrote:
> On 2022/12/9 5:09, Linus Walleij wrote:

> >> +       priv->syscon_offset = args.args[0];
> >> +       priv->syscon_shift  = args.args[1];
> >> +       priv->syscon_mask   = args.args[2];
> >
> > Why should these three things be in the device tree instead of being derived
> > from the compatible-string or just plain hard-coded as #defines?
> > I don't get it.
> >
> Hi Linus,
>
> I'm sorry to bother you, but as for the definition of syscon, after discussing with
> my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in
> the device tree, and the code compatibility is better.

OK sounds good looking forward to seeing the result :)

Yours,
Linus Walleij

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[Shawn Lin]

Hi

On 2022/12/7 21:17, William Qiu wrote:
> Add sdio/emmc driver support for StarFive JH7110 soc.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>   MAINTAINERS                        |   6 +
>   drivers/mmc/host/Kconfig           |  10 ++
>   drivers/mmc/host/Makefile          |   1 +
>   drivers/mmc/host/dw_mmc-starfive.c | 197 +++++++++++++++++++++++++++++
>   4 files changed, 214 insertions(+)
>   create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
> 

...

> +
> +static unsigned long dw_mci_starfive_caps[] = {
> +	MMC_CAP_CMD23,
> +	MMC_CAP_CMD23,
> +	MMC_CAP_CMD23
> +};
> +

....

> +	host->priv = priv;
> +
> +	return 0;
> +}
> +
> +static const struct dw_mci_drv_data starfive_data = {
> +	.caps = dw_mci_starfive_caps,
> +	.num_caps = ARRAY_SIZE(dw_mci_starfive_caps),

use ".common_caps = MMC_CAP_CMD23" instead.

> +	.set_ios = dw_mci_starfive_set_ios,
> +	.parse_dt = dw_mci_starfive_parse_dt,
> +	.execute_tuning = dw_mci_starfive_execute_tuning,
> +};
> +
> +static const struct of_device_id dw_mci_starfive_match[] = {

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[William Qiu]

On 2022/12/13 10:24, Shawn Lin wrote:
> Hi
> 
> On 2022/12/7 21:17, William Qiu wrote:
>> Add sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>   MAINTAINERS                        |   6 +
>>   drivers/mmc/host/Kconfig           |  10 ++
>>   drivers/mmc/host/Makefile          |   1 +
>>   drivers/mmc/host/dw_mmc-starfive.c | 197 +++++++++++++++++++++++++++++
>>   4 files changed, 214 insertions(+)
>>   create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
>>
> 
> ...
> 
>> +
>> +static unsigned long dw_mci_starfive_caps[] = {
>> +    MMC_CAP_CMD23,
>> +    MMC_CAP_CMD23,
>> +    MMC_CAP_CMD23
>> +};
>> +
> 
> ....
> 
>> +    host->priv = priv;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dw_mci_drv_data starfive_data = {
>> +    .caps = dw_mci_starfive_caps,
>> +    .num_caps = ARRAY_SIZE(dw_mci_starfive_caps),
> 
> use ".common_caps = MMC_CAP_CMD23" instead.
> 

Hi Shawn,

Thank you for taking time to review.
The .common_caps is not defined in dw_mci_drv_data.
And .num_caps is also used in dw_mci-rockchip.c.

Best regards,
William Qiu
>> +    .set_ios = dw_mci_starfive_set_ios,
>> +    .parse_dt = dw_mci_starfive_parse_dt,
>> +    .execute_tuning = dw_mci_starfive_execute_tuning,
>> +};
>> +
>> +static const struct of_device_id dw_mci_starfive_match[] = {
> 

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[Shawn Lin]

Hi

On 2022/12/13 15:21, William Qiu wrote:
> 
> 
> On 2022/12/13 10:24, Shawn Lin wrote:
>> Hi
>
>> use ".common_caps = MMC_CAP_CMD23" instead.
>>
> 
> Hi Shawn,
> 
> Thank you for taking time to review.
> The .common_caps is not defined in dw_mci_drv_data.
> And .num_caps is also used in dw_mci-rockchip.c.
> 

That means your patch is based on old upstream kernel which hasn't
been updated for at least one year.:)

> Best regards,
> William Qiu
>>> +    .set_ios = dw_mci_starfive_set_ios,
>>> +    .parse_dt = dw_mci_starfive_parse_dt,
>>> +    .execute_tuning = dw_mci_starfive_execute_tuning,
>>> +};
>>> +
>>> +static const struct of_device_id dw_mci_starfive_match[] = {
>>

Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[William Qiu]

On 2022/12/13 15:33, Shawn Lin wrote:
> Hi
> 
> On 2022/12/13 15:21, William Qiu wrote:
>>
>>
>> On 2022/12/13 10:24, Shawn Lin wrote:
>>> Hi
>>
>>> use ".common_caps = MMC_CAP_CMD23" instead.
>>>
>>
>> Hi Shawn,
>>
>> Thank you for taking time to review.
>> The .common_caps is not defined in dw_mci_drv_data.
>> And .num_caps is also used in dw_mci-rockchip.c.
>>
> 
> That means your patch is based on old upstream kernel which hasn't
> been updated for at least one year.:)
> 

Sorry about that. I just check the code on 5.15.
I will update it in next version.
Thank you any way.

>> Best regards,
>> William Qiu
>>>> +    .set_ios = dw_mci_starfive_set_ios,
>>>> +    .parse_dt = dw_mci_starfive_parse_dt,
>>>> +    .execute_tuning = dw_mci_starfive_execute_tuning,
>>>> +};
>>>> +
>>>> +static const struct of_device_id dw_mci_starfive_match[] = {
>>>

[PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[William Qiu]

This adds the mmc node for the StarFive JH7110 SoC.
Set sdioo node to emmc and set sdio1 node to sd.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
index c8946cf3a268..6ef8e303c2e6 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
@@ -47,6 +47,31 @@ &clk_rtc {
 	clock-frequency = <32768>;
 };
 
+&sdio0 {
+	max-frequency = <100000000>;
+	card-detect-delay = <300>;
+	bus-width = <8>;
+	cap-mmc-highspeed;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
+	non-removable;
+	cap-mmc-hw-reset;
+	post-power-on-delay-ms = <200>;
+	status = "okay";
+};
+
+&sdio1 {
+	max-frequency = <100000000>;
+	card-detect-delay = <300>;
+	bus-width = <4>;
+	no-sdio;
+	no-mmc;
+	broken-cd;
+	cap-sd-highspeed;
+	post-power-on-delay-ms = <200>;
+	status = "okay";
+};
+
 &gmac0_rmii_refin {
 	clock-frequency = <50000000>;
 };
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index c22e8f1d2640..e90b085d7e41 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
 			#reset-cells = <1>;
 		};
 
+		sys_syscon: sys_syscon@13030000 {
+			compatible = "syscon";
+			reg = <0x0 0x13030000 0x0 0x1000>;
+		};
+
 		gpio: gpio@13040000 {
 			compatible = "starfive,jh7110-sys-pinctrl";
 			reg = <0x0 0x13040000 0x0 0x10000>;
@@ -433,5 +438,38 @@ uart5: serial@12020000 {
 			reg-shift = <2>;
 			status = "disabled";
 		};
+
+		/* unremovable emmc as mmcblk0 */
+		sdio0: mmc@16010000 {
+			compatible = "starfive,jh7110-sdio";
+			reg = <0x0 0x16010000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
+				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
+			clock-names = "biu","ciu";
+			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
+			reset-names = "reset";
+			interrupts = <74>;
+			fifo-depth = <32>;
+			fifo-watermark-aligned;
+			data-addr = <0>;
+			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
+			status = "disabled";
+		};
+
+		sdio1: mmc@16020000 {
+			compatible = "starfive,jh7110-sdio";
+			reg = <0x0 0x16020000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_SDIO1_AHB>,
+				 <&syscrg JH7110_SYSCLK_SDIO1_SDCARD>;
+			clock-names = "biu","ciu";
+			resets = <&syscrg JH7110_SYSRST_SDIO1_AHB>;
+			reset-names = "reset";
+			interrupts = <75>;
+			fifo-depth = <32>;
+			fifo-watermark-aligned;
+			data-addr = <0>;
+			starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;
+			status = "disabled";
+		};
 	};
 };
-- 
2.34.1

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[Krzysztof Kozlowski]

On 07/12/2022 14:17, William Qiu wrote:
> This adds the mmc node for the StarFive JH7110 SoC.
> Set sdioo node to emmc and set sdio1 node to sd.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> index c8946cf3a268..6ef8e303c2e6 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> @@ -47,6 +47,31 @@ &clk_rtc {
>  	clock-frequency = <32768>;
>  };
>  
> +&sdio0 {
> +	max-frequency = <100000000>;
> +	card-detect-delay = <300>;
> +	bus-width = <8>;
> +	cap-mmc-highspeed;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	non-removable;
> +	cap-mmc-hw-reset;
> +	post-power-on-delay-ms = <200>;
> +	status = "okay";
> +};
> +
> +&sdio1 {
> +	max-frequency = <100000000>;
> +	card-detect-delay = <300>;
> +	bus-width = <4>;
> +	no-sdio;
> +	no-mmc;
> +	broken-cd;
> +	cap-sd-highspeed;
> +	post-power-on-delay-ms = <200>;
> +	status = "okay";
> +};
> +
>  &gmac0_rmii_refin {
>  	clock-frequency = <50000000>;
>  };
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index c22e8f1d2640..e90b085d7e41 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
>  			#reset-cells = <1>;
>  		};
>  
> +		sys_syscon: sys_syscon@13030000 {

No underscores in node names, generic node names (syscon or
system-controller)

> +			compatible = "syscon";

This is not allowed alone.

> +			reg = <0x0 0x13030000 0x0 0x1000>;
> +		};
> +
>  		gpio: gpio@13040000 {
>  			compatible = "starfive,jh7110-sys-pinctrl";
>  			reg = <0x0 0x13040000 0x0 0x10000>;
> @@ -433,5 +438,38 @@ uart5: serial@12020000 {
>  			reg-shift = <2>;
>  			status = "disabled";
>  		};
> +
> +		/* unremovable emmc as mmcblk0 */
> +		sdio0: mmc@16010000 {
> +			compatible = "starfive,jh7110-sdio";
> +			reg = <0x0 0x16010000 0x0 0x10000>;
> +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
> +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
> +			clock-names = "biu","ciu";
> +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
> +			reset-names = "reset";
> +			interrupts = <74>;
> +			fifo-depth = <32>;
> +			fifo-watermark-aligned;
> +			data-addr = <0>;
> +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;

This does not match your bindings at all. "&sys_syscon" is a phandle,
not a number of tuning retries, as you expect in your bindings.


Best regards,
Krzysztof

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4324 bytes --]

On Wed, Dec 07, 2022 at 04:14:53PM +0100, Krzysztof Kozlowski wrote:
> On 07/12/2022 14:17, William Qiu wrote:
> > This adds the mmc node for the StarFive JH7110 SoC.
> > Set sdioo node to emmc and set sdio1 node to sd.
> > 
> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > ---
> >  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > index c8946cf3a268..6ef8e303c2e6 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > @@ -47,6 +47,31 @@ &clk_rtc {
> >  	clock-frequency = <32768>;
> >  };
> >  
> > +&sdio0 {
> > +	max-frequency = <100000000>;
> > +	card-detect-delay = <300>;
> > +	bus-width = <8>;
> > +	cap-mmc-highspeed;
> > +	mmc-ddr-1_8v;
> > +	mmc-hs200-1_8v;
> > +	non-removable;
> > +	cap-mmc-hw-reset;
> > +	post-power-on-delay-ms = <200>;
> > +	status = "okay";
> > +};
> > +
> > +&sdio1 {
> > +	max-frequency = <100000000>;
> > +	card-detect-delay = <300>;
> > +	bus-width = <4>;
> > +	no-sdio;
> > +	no-mmc;
> > +	broken-cd;
> > +	cap-sd-highspeed;
> > +	post-power-on-delay-ms = <200>;
> > +	status = "okay";
> > +};
> > +
> >  &gmac0_rmii_refin {
> >  	clock-frequency = <50000000>;
> >  };
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > index c22e8f1d2640..e90b085d7e41 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
> >  			#reset-cells = <1>;
> >  		};
> >  
> > +		sys_syscon: sys_syscon@13030000 {
> 
> No underscores in node names, generic node names (syscon or
> system-controller)
> 
> > +			compatible = "syscon";
> 
> This is not allowed alone.
> 
> > +			reg = <0x0 0x13030000 0x0 0x1000>;
> > +		};
> > +
> >  		gpio: gpio@13040000 {
> >  			compatible = "starfive,jh7110-sys-pinctrl";
> >  			reg = <0x0 0x13040000 0x0 0x10000>;
> > @@ -433,5 +438,38 @@ uart5: serial@12020000 {
> >  			reg-shift = <2>;
> >  			status = "disabled";
> >  		};
> > +
> > +		/* unremovable emmc as mmcblk0 */
> > +		sdio0: mmc@16010000 {
> > +			compatible = "starfive,jh7110-sdio";
> > +			reg = <0x0 0x16010000 0x0 0x10000>;
> > +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
> > +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
> > +			clock-names = "biu","ciu";
> > +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
> > +			reset-names = "reset";
> > +			interrupts = <74>;
> > +			fifo-depth = <32>;
> > +			fifo-watermark-aligned;
> > +			data-addr = <0>;
> > +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> 
> This does not match your bindings at all. "&sys_syscon" is a phandle,
> not a number of tuning retries, as you expect in your bindings.

Additionally, a Link: to the documentation for where-ever these "random"
numbers that are being used would be nice.

+static int dw_mci_starfive_parse_dt(struct dw_mci *host)
+{
+	struct of_phandle_args args;
+	struct starfive_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
+						"starfive,sys-syscon", 3, 0, &args);
+	if (ret) {
+		dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
+		return -EINVAL;
+	}
+
+	priv->reg_syscon = syscon_node_to_regmap(args.np);
+	of_node_put(args.np);
+	if (IS_ERR(priv->reg_syscon))
+		return PTR_ERR(priv->reg_syscon);
+
+	priv->syscon_offset = args.args[0];
+	priv->syscon_shift  = args.args[1];
+	priv->syscon_mask   = args.args[2];

Given the driver, the property description just seems incorrect and this
is actually the bit of the syscon that is relevant to the tuning process
(perhaps where the find the tuning values?). Without public docs or a
better description it is hard for (me at least) to know :)

+
+	host->priv = priv;
+
+	return 0;
+}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[William Qiu]

On 2022/12/8 0:31, Conor Dooley wrote:
> On Wed, Dec 07, 2022 at 04:14:53PM +0100, Krzysztof Kozlowski wrote:
>> On 07/12/2022 14:17, William Qiu wrote:
>> > This adds the mmc node for the StarFive JH7110 SoC.
>> > Set sdioo node to emmc and set sdio1 node to sd.
>> > 
>> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> > ---
>> >  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
>> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
>> >  2 files changed, 63 insertions(+)
>> > 
>> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> > index c8946cf3a268..6ef8e303c2e6 100644
>> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> > @@ -47,6 +47,31 @@ &clk_rtc {
>> >  	clock-frequency = <32768>;
>> >  };
>> >  
>> > +&sdio0 {
>> > +	max-frequency = <100000000>;
>> > +	card-detect-delay = <300>;
>> > +	bus-width = <8>;
>> > +	cap-mmc-highspeed;
>> > +	mmc-ddr-1_8v;
>> > +	mmc-hs200-1_8v;
>> > +	non-removable;
>> > +	cap-mmc-hw-reset;
>> > +	post-power-on-delay-ms = <200>;
>> > +	status = "okay";
>> > +};
>> > +
>> > +&sdio1 {
>> > +	max-frequency = <100000000>;
>> > +	card-detect-delay = <300>;
>> > +	bus-width = <4>;
>> > +	no-sdio;
>> > +	no-mmc;
>> > +	broken-cd;
>> > +	cap-sd-highspeed;
>> > +	post-power-on-delay-ms = <200>;
>> > +	status = "okay";
>> > +};
>> > +
>> >  &gmac0_rmii_refin {
>> >  	clock-frequency = <50000000>;
>> >  };
>> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > index c22e8f1d2640..e90b085d7e41 100644
>> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
>> >  			#reset-cells = <1>;
>> >  		};
>> >  
>> > +		sys_syscon: sys_syscon@13030000 {
>> 
>> No underscores in node names, generic node names (syscon or
>> system-controller)
>> 
>> > +			compatible = "syscon";
>> 
>> This is not allowed alone.
>> 
>> > +			reg = <0x0 0x13030000 0x0 0x1000>;
>> > +		};
>> > +
>> >  		gpio: gpio@13040000 {
>> >  			compatible = "starfive,jh7110-sys-pinctrl";
>> >  			reg = <0x0 0x13040000 0x0 0x10000>;
>> > @@ -433,5 +438,38 @@ uart5: serial@12020000 {
>> >  			reg-shift = <2>;
>> >  			status = "disabled";
>> >  		};
>> > +
>> > +		/* unremovable emmc as mmcblk0 */
>> > +		sdio0: mmc@16010000 {
>> > +			compatible = "starfive,jh7110-sdio";
>> > +			reg = <0x0 0x16010000 0x0 0x10000>;
>> > +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
>> > +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
>> > +			clock-names = "biu","ciu";
>> > +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
>> > +			reset-names = "reset";
>> > +			interrupts = <74>;
>> > +			fifo-depth = <32>;
>> > +			fifo-watermark-aligned;
>> > +			data-addr = <0>;
>> > +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
>> 
>> This does not match your bindings at all. "&sys_syscon" is a phandle,
>> not a number of tuning retries, as you expect in your bindings.
> 
> Additionally, a Link: to the documentation for where-ever these "random"
> numbers that are being used would be nice.
> 

I will update in next version.

> +static int dw_mci_starfive_parse_dt(struct dw_mci *host)
> +{
> +	struct of_phandle_args args;
> +	struct starfive_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
> +						"starfive,sys-syscon", 3, 0, &args);
> +	if (ret) {
> +		dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->reg_syscon = syscon_node_to_regmap(args.np);
> +	of_node_put(args.np);
> +	if (IS_ERR(priv->reg_syscon))
> +		return PTR_ERR(priv->reg_syscon);
> +
> +	priv->syscon_offset = args.args[0];
> +	priv->syscon_shift  = args.args[1];
> +	priv->syscon_mask   = args.args[2];
> 
> Given the driver, the property description just seems incorrect and this
> is actually the bit of the syscon that is relevant to the tuning process
> (perhaps where the find the tuning values?). Without public docs or a
> better description it is hard for (me at least) to know :)
> 

Hi Conor,

Thank you for taking time to review and provide helpful comments for this patch.
I'll make the description better and more detailed in next version.

Best regards,
William Qiu
> +
> +	host->priv = priv;
> +
> +	return 0;
> +}

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[William Qiu]

On 2022/12/7 23:14, Krzysztof Kozlowski wrote:
> On 07/12/2022 14:17, William Qiu wrote:
>> This adds the mmc node for the StarFive JH7110 SoC.
>> Set sdioo node to emmc and set sdio1 node to sd.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
>>  2 files changed, 63 insertions(+)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> index c8946cf3a268..6ef8e303c2e6 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> @@ -47,6 +47,31 @@ &clk_rtc {
>>  	clock-frequency = <32768>;
>>  };
>>  
>> +&sdio0 {
>> +	max-frequency = <100000000>;
>> +	card-detect-delay = <300>;
>> +	bus-width = <8>;
>> +	cap-mmc-highspeed;
>> +	mmc-ddr-1_8v;
>> +	mmc-hs200-1_8v;
>> +	non-removable;
>> +	cap-mmc-hw-reset;
>> +	post-power-on-delay-ms = <200>;
>> +	status = "okay";
>> +};
>> +
>> +&sdio1 {
>> +	max-frequency = <100000000>;
>> +	card-detect-delay = <300>;
>> +	bus-width = <4>;
>> +	no-sdio;
>> +	no-mmc;
>> +	broken-cd;
>> +	cap-sd-highspeed;
>> +	post-power-on-delay-ms = <200>;
>> +	status = "okay";
>> +};
>> +
>>  &gmac0_rmii_refin {
>>  	clock-frequency = <50000000>;
>>  };
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index c22e8f1d2640..e90b085d7e41 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
>>  			#reset-cells = <1>;
>>  		};
>>  
>> +		sys_syscon: sys_syscon@13030000 {
> 
> No underscores in node names, generic node names (syscon or
> system-controller)

Will fix.

> 
>> +			compatible = "syscon";
> 
> This is not allowed alone.

Will fix.

> 
>> +			reg = <0x0 0x13030000 0x0 0x1000>;
>> +		};
>> +
>>  		gpio: gpio@13040000 {
>>  			compatible = "starfive,jh7110-sys-pinctrl";
>>  			reg = <0x0 0x13040000 0x0 0x10000>;
>> @@ -433,5 +438,38 @@ uart5: serial@12020000 {
>>  			reg-shift = <2>;
>>  			status = "disabled";
>>  		};
>> +
>> +		/* unremovable emmc as mmcblk0 */
>> +		sdio0: mmc@16010000 {
>> +			compatible = "starfive,jh7110-sdio";
>> +			reg = <0x0 0x16010000 0x0 0x10000>;
>> +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
>> +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
>> +			clock-names = "biu","ciu";
>> +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
>> +			reset-names = "reset";
>> +			interrupts = <74>;
>> +			fifo-depth = <32>;
>> +			fifo-watermark-aligned;
>> +			data-addr = <0>;
>> +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> 
> This does not match your bindings at all. "&sys_syscon" is a phandle,
> not a number of tuning retries, as you expect in your bindings.
> 

Hi Krzysztof,

The sys_syscon is one of system controller registers, and it is used to be the data strobe
delay chain select. I will update its type and description in next version.

Thanks,
William Qiu

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[Linus Walleij]

Hi William,

thanks for your patch!

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

> +                       starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
(...)
> +                       starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;

These are registster offsets and shifts. Don't define such stuff in your device
tree, use #defines in the code and just provide the phandle.

Yours,
Linus Walleij

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[William Qiu]

On 2022/12/9 5:15, 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:
> 
>> +                       starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> (...)
>> +                       starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;
> 
> These are registster offsets and shifts. Don't define such stuff in your device
> tree, use #defines in the code and just provide the phandle.
> 

Hi Linus,

I will use #define in code instead in device tree.
Thank you for taking time to review and provide helpful comments for this patch.

Best Regards,
William Qiu
> Yours,
> Linus Walleij

Re: [PATCH v1 3/3] riscv: dts: starfive: Add mmc node

[William Qiu]

On 2022/12/9 5:15, 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:
> 
>> +                       starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> (...)
>> +                       starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;
> 
> These are registster offsets and shifts. Don't define such stuff in your device
> tree, use #defines in the code and just provide the phandle.
> 
Hi Linus,

I'm sorry to bother you, but as for the definition of syscon, after discussing with 
my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in
the device tree, and the code compatibility is better. I expected to discuss this issue
with you in V2,, but I am not sure whether you have seen V2, so I can only ask for
your advice in V1.

Best Regards
William Qiu
> Yours,
> Linus Walleij

Re: [PATCH v1 0/3] StarFive's SDIO/eMMC driver support

[William Qiu]

On 2022/12/7 21:17, William Qiu wrote:
> Hi,
> 
> This patchset adds initial rudimentary support for the StarFive
> designware mobile storage host controller driver. And this driver will
> be used in StarFive's visionfive-v2 board. The main purpose of adding
> this driver is to accommodate the ultra-high speed mode of eMMC.
> 
> The patch series is based on v6.1-rc5.
> 
> -- William
> 
> William Qiu (3):
>   dt-bindings: mmc: Add bindings for StarFive
>   mmc: starfive: Add sdio/emmc driver support
>   riscv: dts: starfive: Add mmc node
> 
>  .../bindings/mmc/starfive,jh7110-sdio.yaml    |  71 +++++++
>  MAINTAINERS                                   |   6 +
>  .../jh7110-starfive-visionfive-v2.dts         |  25 +++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      |  38 ++++
>  drivers/mmc/host/Kconfig                      |  10 +
>  drivers/mmc/host/Makefile                     |   1 +
>  drivers/mmc/host/dw_mmc-starfive.c            | 197 ++++++++++++++++++
>  7 files changed, 348 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>  create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
> 
> --
> 2.34.1
> 

Hi Jaehoon/Ulf,

Could you please help to review and provide comments on this patch series?
Thank you in advance.

Best regards,
William Qiu

Re: [PATCH v1 0/3] StarFive's SDIO/eMMC driver support

[Ulf Hansson]

On Fri, 16 Dec 2022 at 03:02, William Qiu <william.qiu@starfivetech.com> wrote:
>
>
>
> On 2022/12/7 21:17, William Qiu wrote:
> > Hi,
> >
> > This patchset adds initial rudimentary support for the StarFive
> > designware mobile storage host controller driver. And this driver will
> > be used in StarFive's visionfive-v2 board. The main purpose of adding
> > this driver is to accommodate the ultra-high speed mode of eMMC.
> >
> > The patch series is based on v6.1-rc5.
> >
> > -- William
> >
> > William Qiu (3):
> >   dt-bindings: mmc: Add bindings for StarFive
> >   mmc: starfive: Add sdio/emmc driver support
> >   riscv: dts: starfive: Add mmc node
> >
> >  .../bindings/mmc/starfive,jh7110-sdio.yaml    |  71 +++++++
> >  MAINTAINERS                                   |   6 +
> >  .../jh7110-starfive-visionfive-v2.dts         |  25 +++
> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      |  38 ++++
> >  drivers/mmc/host/Kconfig                      |  10 +
> >  drivers/mmc/host/Makefile                     |   1 +
> >  drivers/mmc/host/dw_mmc-starfive.c            | 197 ++++++++++++++++++
> >  7 files changed, 348 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> >  create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
> >
> > --
> > 2.34.1
> >
>
> Hi Jaehoon/Ulf,
>
> Could you please help to review and provide comments on this patch series?
> Thank you in advance.

Hi William,

Looks like you have received plenty of good comments already and there
are a lot of things for you to update. That said, I think it makes
better sense for me to look at the next version instead.

>
> Best regards,
> William Qiu

Kind regards
Uffe

Re: [PATCH v1 0/3] StarFive's SDIO/eMMC driver support

[William Qiu]

On 2022/12/16 17:22, Ulf Hansson wrote:
> On Fri, 16 Dec 2022 at 03:02, William Qiu <william.qiu@starfivetech.com> wrote:
>>
>>
>>
>> On 2022/12/7 21:17, William Qiu wrote:
>> > Hi,
>> >
>> > This patchset adds initial rudimentary support for the StarFive
>> > designware mobile storage host controller driver. And this driver will
>> > be used in StarFive's visionfive-v2 board. The main purpose of adding
>> > this driver is to accommodate the ultra-high speed mode of eMMC.
>> >
>> > The patch series is based on v6.1-rc5.
>> >
>> > -- William
>> >
>> > William Qiu (3):
>> >   dt-bindings: mmc: Add bindings for StarFive
>> >   mmc: starfive: Add sdio/emmc driver support
>> >   riscv: dts: starfive: Add mmc node
>> >
>> >  .../bindings/mmc/starfive,jh7110-sdio.yaml    |  71 +++++++
>> >  MAINTAINERS                                   |   6 +
>> >  .../jh7110-starfive-visionfive-v2.dts         |  25 +++
>> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      |  38 ++++
>> >  drivers/mmc/host/Kconfig                      |  10 +
>> >  drivers/mmc/host/Makefile                     |   1 +
>> >  drivers/mmc/host/dw_mmc-starfive.c            | 197 ++++++++++++++++++
>> >  7 files changed, 348 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> >  create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
>> >
>> > --
>> > 2.34.1
>> >
>>
>> Hi Jaehoon/Ulf,
>>
>> Could you please help to review and provide comments on this patch series?
>> Thank you in advance.
> 
> Hi William,
> 
> Looks like you have received plenty of good comments already and there
> are a lot of things for you to update. That said, I think it makes
> better sense for me to look at the next version instead.
> 

Fine, I'll do that then.

Thanks
William Qiu

>>
>> Best regards,
>> William Qiu
> 
> Kind regards
> Uffe