mbox series

[V7,0/4] Add DesignWare Mobile mmc driver

Message ID 20231121091101.5540-1-jyanchou@realtek.com
Headers show
Series Add DesignWare Mobile mmc driver | expand

Message

Jyan Chou [周芷安] Nov. 21, 2023, 9:10 a.m. UTC 
We added Synopsys DesignWare mmc cmdq driver and also added
Realtek mmc driver to make good use of it.

Jyan Chou (4):
  mmc: solve DMA boundary limitation of CQHCI driver
  mmc: Add Synopsys DesignWare mmc cmdq host driver
  mmc: Add dw mobile mmc cmdq rtk driver
  dt-bindings: mmc: Add dt-bindings for realtek mmc driver

 .../bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml |  153 ++
 drivers/mmc/host/Kconfig                      |   22 +
 drivers/mmc/host/Makefile                     |    2 +
 drivers/mmc/host/cqhci-core.c                 |   13 +-
 drivers/mmc/host/cqhci.h                      |    8 +
 drivers/mmc/host/dw_mmc_cqe-rtk.c             |  768 +++++++++
 drivers/mmc/host/dw_mmc_cqe-rtk.h             |  160 ++
 drivers/mmc/host/dw_mmc_cqe.c                 | 1467 +++++++++++++++++
 drivers/mmc/host/dw_mmc_cqe.h                 |  456 +++++
 9 files changed, 3046 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml
 create mode 100644 drivers/mmc/host/dw_mmc_cqe-rtk.c
 create mode 100644 drivers/mmc/host/dw_mmc_cqe-rtk.h
 create mode 100644 drivers/mmc/host/dw_mmc_cqe.c
 create mode 100644 drivers/mmc/host/dw_mmc_cqe.h

Comments

Krzysztof Kozlowski Nov. 21, 2023, 9:35 a.m. UTC | #1 
On 21/11/2023 10:11, Jyan Chou wrote:
> Document the device-tree bindings for Realtek SoCs mmc driver.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.

Additionally:

A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.

> 
> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
> 
> ---
> v6 -> v7:
> - Drop reset-names and realtek,m2tmx since it is not needed in our driver.
> 
> v5 -> v6:
> - Drop the incorrect, generic compatible and modify it to specific.
> - Drop useless properties.

I asked which ones. I still do not see that information here.


> - Modify bindings to make DTS and driver match.
> 
> v4 -> v5:
> - Remove unused property, e.g.,cqe, resets, clock-freq-min-max.
> - Fix indentation.
> 

...

> +
> +maintainers:
> +  - Jyan Chou <jyanchou@realtek.com>
> +
> +allOf:
> +  - $ref: synopsys-dw-mshc-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd1325-dw-cqe-emmc
> +      - realtek,rtd1319-dw-cqe-emmc
> +      - realtek,rtd1315e-dw-cqe-emmc
> +      - realtek,rtd1619b-dw-cqe-emmc

That's not what is present in your driver. Why other compatibles are not
there? What do you want to express here?

> +
> +  reg:
> +    items:
> +      - description: emmc base address
> +      - description: cqhci base address
> +
> +  reg-names:
> +    items:
> +      - const: emmc
> +      - const: cqhci
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: biu
> +      - const: ciu
> +      - const: vp0
> +      - const: vp1
> +
> +  resets:
> +    maxItems: 1
> +
> +  pinctrl-0:
> +    description:
> +      should contain default/high speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    description:
> +      should contain sdr50 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-2:
> +    description:
> +      should contain ddr50 mode pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-3:
> +    description:
> +      should contain hs200 speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-4:
> +    description:
> +      should contain hs400 speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-5:
> +    description:
> +      should contain tune0 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-6:
> +    description:
> +      should contain tune1 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-7:
> +    description:
> +      should contain tune2 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-8:
> +    description:
> +      should contain tune3 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-9:
> +    description:
> +      should contain tune4 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-names:
> +    maxItems: 10
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - vmmc-supply
> +  - pinctrl-names
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-3
> +  - pinctrl-4
> +  - pinctrl-5
> +  - pinctrl-6
> +  - pinctrl-7
> +  - pinctrl-8
> +  - pinctrl-9
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    emmc: mmc@12000 {
> +      compatible = "realtek,rtd1315e-dw-cqe-emmc";
> +      reg = <0x00012000 0x00600>,
> +            <0x00012180 0x00060>;
> +      reg-names = "emmc", "cqhci";
> +      interrupts = <0 42 4>;

Use proper interrupt defines instead of raw values.

> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
> +      clock-names = "biu", "ciu", "vp0", "vp1";

Where is the DTS implementing this binding?

I said it once:

> I asked you to test the bindings. This also means that you must test
> your DTS against bindings. Your bindings, DTS and driver do not match,
> therefore let's be a bit more clear:
>
> NAK, till you upstream your DTS.

I still do not see DTS anywhere. Please link it.

Best regards,
Krzysztof
Jyan Chou [周芷安] Nov. 22, 2023, 2:54 a.m. UTC | #2 
>> Document the device-tree bindings for Realtek SoCs mmc driver.

> This is a friendly reminder during the review process.

> It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.

> Thank you.

> Additionally:

> A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings.

Okay, I will check if I miss to correct some previous comments.

>>
>> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
>>
>> ---
>> v6 -> v7:
>> - Drop reset-names and realtek,m2tmx since it is not needed in our driver.
>>
>> v5 -> v6:
>> - Drop the incorrect, generic compatible and modify it to specific.
>> - Drop useless properties.

> I asked which ones. I still do not see that information here.

Okay, I will list it in.

>> - Modify bindings to make DTS and driver match.
>>
>> v4 -> v5:
>> - Remove unused property, e.g.,cqe, resets, clock-freq-min-max.
>> - Fix indentation.
>>

...

>> +
>> +maintainers:
>> +  - Jyan Chou <jyanchou@realtek.com>
>> +
>> +allOf:
>> +  - $ref: synopsys-dw-mshc-common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - realtek,rtd1325-dw-cqe-emmc
>> +      - realtek,rtd1319-dw-cqe-emmc
>> +      - realtek,rtd1315e-dw-cqe-emmc
>> +      - realtek,rtd1619b-dw-cqe-emmc

> That's not what is present in your driver. Why other compatibles are not there? What do you want to express here?

Other compatibles are surely needed to be in our driver, I will correct it in our new version, thanks.

>> +
>> +  reg:
>> +    items:
>> +      - description: emmc base address
>> +      - description: cqhci base address
>> +
>> +  reg-names:
>> +    items:
>> +      - const: emmc
>> +      - const: cqhci
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    items:
>> +      - const: biu
>> +      - const: ciu
>> +      - const: vp0
>> +      - const: vp1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  pinctrl-0:
>> +    description:
>> +      should contain default/high speed pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-1:
>> +    description:
>> +      should contain sdr50 pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-2:
>> +    description:
>> +      should contain ddr50 mode pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-3:
>> +    description:
>> +      should contain hs200 speed pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-4:
>> +    description:
>> +      should contain hs400 speed pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-5:
>> +    description:
>> +      should contain tune0 pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-6:
>> +    description:
>> +      should contain tune1 pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-7:
>> +    description:
>> +      should contain tune2 pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-8:
>> +    description:
>> +      should contain tune3 pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-9:
>> +    description:
>> +      should contain tune4 pin ctrl.
>> +    maxItems: 1
>> +
>> +  pinctrl-names:
>> +    maxItems: 10
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - vmmc-supply
>> +  - pinctrl-names
>> +  - pinctrl-0
>> +  - pinctrl-1
>> +  - pinctrl-3
>> +  - pinctrl-4
>> +  - pinctrl-5
>> +  - pinctrl-6
>> +  - pinctrl-7
>> +  - pinctrl-8
>> +  - pinctrl-9
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    emmc: mmc@12000 {
>> +      compatible = "realtek,rtd1315e-dw-cqe-emmc";
>> +      reg = <0x00012000 0x00600>,
>> +            <0x00012180 0x00060>;
>> +      reg-names = "emmc", "cqhci";
>> +      interrupts = <0 42 4>;

> Use proper interrupt defines instead of raw values.

Okay, I will correct it.

> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
> +      clock-names = "biu", "ciu", "vp0", "vp1";

> Where is the DTS implementing this binding?

> I said it once:

>> I asked you to test the bindings. This also means that you must test 
>> your DTS against bindings. Your bindings, DTS and driver do not match, 
>> therefore let's be a bit more clear:
>>
>> NAK, till you upstream your DTS.

> I still do not see DTS anywhere. Please link it.

Sorry for asking. Which dts do we need to link it? The device tree we use for our SOC?

or realtek,rtd-dw-cqe-emmc.example.dts?  Thanks.


Best Regards,

Jyan
-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: Tuesday, November 21, 2023 5:35 PM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>; ulf.hansson@linaro.org; adrian.hunter@intel.com; jh80.chung@samsung.com; riteshh@codeaurora.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org
Cc: conor+dt@kernel.org; asutoshd@codeaurora.org; p.zabel@pengutronix.de; linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; arnd@arndb.de; briannorris@chromium.org; doug@schmorgal.com; tonyhuang.sunplus@gmail.com; abel.vesa@linaro.org; william.qiu@starfivetech.com
Subject: Re: [PATCH v7][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver


External mail.



On 21/11/2023 10:11, Jyan Chou wrote:
> Document the device-tree bindings for Realtek SoCs mmc driver.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.

Thank you.

Additionally:

A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings.

>
> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
>
> ---
> v6 -> v7:
> - Drop reset-names and realtek,m2tmx since it is not needed in our driver.
>
> v5 -> v6:
> - Drop the incorrect, generic compatible and modify it to specific.
> - Drop useless properties.

I asked which ones. I still do not see that information here.


> - Modify bindings to make DTS and driver match.
>
> v4 -> v5:
> - Remove unused property, e.g.,cqe, resets, clock-freq-min-max.
> - Fix indentation.
>

...

> +
> +maintainers:
> +  - Jyan Chou <jyanchou@realtek.com>
> +
> +allOf:
> +  - $ref: synopsys-dw-mshc-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd1325-dw-cqe-emmc
> +      - realtek,rtd1319-dw-cqe-emmc
> +      - realtek,rtd1315e-dw-cqe-emmc
> +      - realtek,rtd1619b-dw-cqe-emmc

That's not what is present in your driver. Why other compatibles are not there? What do you want to express here?

> +
> +  reg:
> +    items:
> +      - description: emmc base address
> +      - description: cqhci base address
> +
> +  reg-names:
> +    items:
> +      - const: emmc
> +      - const: cqhci
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: biu
> +      - const: ciu
> +      - const: vp0
> +      - const: vp1
> +
> +  resets:
> +    maxItems: 1
> +
> +  pinctrl-0:
> +    description:
> +      should contain default/high speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    description:
> +      should contain sdr50 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-2:
> +    description:
> +      should contain ddr50 mode pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-3:
> +    description:
> +      should contain hs200 speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-4:
> +    description:
> +      should contain hs400 speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-5:
> +    description:
> +      should contain tune0 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-6:
> +    description:
> +      should contain tune1 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-7:
> +    description:
> +      should contain tune2 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-8:
> +    description:
> +      should contain tune3 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-9:
> +    description:
> +      should contain tune4 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-names:
> +    maxItems: 10
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - vmmc-supply
> +  - pinctrl-names
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-3
> +  - pinctrl-4
> +  - pinctrl-5
> +  - pinctrl-6
> +  - pinctrl-7
> +  - pinctrl-8
> +  - pinctrl-9
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    emmc: mmc@12000 {
> +      compatible = "realtek,rtd1315e-dw-cqe-emmc";
> +      reg = <0x00012000 0x00600>,
> +            <0x00012180 0x00060>;
> +      reg-names = "emmc", "cqhci";
> +      interrupts = <0 42 4>;

Use proper interrupt defines instead of raw values.

> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
> +      clock-names = "biu", "ciu", "vp0", "vp1";

Where is the DTS implementing this binding?

I said it once:

> I asked you to test the bindings. This also means that you must test 
> your DTS against bindings. Your bindings, DTS and driver do not match, 
> therefore let's be a bit more clear:
>
> NAK, till you upstream your DTS.

I still do not see DTS anywhere. Please link it.

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 22, 2023, 7:36 a.m. UTC | #3 
On 22/11/2023 03:54, Jyan Chou [周芷安] wrote:
>> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
>> +      clock-names = "biu", "ciu", "vp0", "vp1";
> 
>> Where is the DTS implementing this binding?
> 
>> I said it once:
> 
>>> I asked you to test the bindings. This also means that you must test 
>>> your DTS against bindings. Your bindings, DTS and driver do not match, 
>>> therefore let's be a bit more clear:
>>>
>>> NAK, till you upstream your DTS.
> 
>> I still do not see DTS anywhere. Please link it.
> 
> Sorry for asking. Which dts do we need to link it? The device tree we use for our SOC?
> 
> or realtek,rtd-dw-cqe-emmc.example.dts?  Thanks.

The patch adding this device node to any upstream DTS. I am not speaking
about example, but about upstream DTS using this MMC.

As I wrote last time, I don't believe you test bindings and DTS and I
had proofs of that. Therefore I expect you to upstream DTS of your product.

Best regards,
Krzysztof
Jyan Chou [周芷安] Nov. 22, 2023, 8:33 a.m. UTC | #4 
>>> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
>>> +      clock-names = "biu", "ciu", "vp0", "vp1";
>>
>> Where is the DTS implementing this binding?
>>
>>> I said it once:
>>
>>>> I asked you to test the bindings. This also means that you must test 
>>>> your DTS against bindings. Your bindings, DTS and driver do not 
>>>> match, therefore let's be a bit more clear:
>>>>
>>>> NAK, till you upstream your DTS.
>>
>>> I still do not see DTS anywhere. Please link it.
>>
>> Sorry for asking. Which dts do we need to link it? The device tree we use for our SOC?
>>
>> or realtek,rtd-dw-cqe-emmc.example.dts?  Thanks.

> The patch adding this device node to any upstream DTS. I am not speaking about example, but about upstream DTS using this MMC.

> As I wrote last time, I don't believe you test bindings and DTS and I had proofs of that. Therefore I expect you to upstream DTS of your product.

Sorry for explaining, since our DTS is still trying to upstream, but not accepted by upstream yet,

https://lore.kernel.org/linux-arm-kernel/4a462b2e-380a-9fd1-2e84-783cc457e8c2@linaro.org/T/

could I provide the dts we local used now for you first to compare with bindings? I am sorry for making this request.

Best regards,
Jyan

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: Wednesday, November 22, 2023 3:37 PM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>; ulf.hansson@linaro.org; adrian.hunter@intel.com; jh80.chung@samsung.com; riteshh@codeaurora.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org
Cc: conor+dt@kernel.org; asutoshd@codeaurora.org; p.zabel@pengutronix.de; linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; arnd@arndb.de; briannorris@chromium.org; doug@schmorgal.com; tonyhuang.sunplus@gmail.com; abel.vesa@linaro.org; william.qiu@starfivetech.com
Subject: Re: [PATCH v7][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver


External mail.



On 22/11/2023 03:54, Jyan Chou [周芷安] wrote:
>> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
>> +      clock-names = "biu", "ciu", "vp0", "vp1";
>
>> Where is the DTS implementing this binding?
>
>> I said it once:
>
>>> I asked you to test the bindings. This also means that you must test 
>>> your DTS against bindings. Your bindings, DTS and driver do not 
>>> match, therefore let's be a bit more clear:
>>>
>>> NAK, till you upstream your DTS.
>
>> I still do not see DTS anywhere. Please link it.
>
> Sorry for asking. Which dts do we need to link it? The device tree we use for our SOC?
>
> or realtek,rtd-dw-cqe-emmc.example.dts?  Thanks.

The patch adding this device node to any upstream DTS. I am not speaking about example, but about upstream DTS using this MMC.

As I wrote last time, I don't believe you test bindings and DTS and I had proofs of that. Therefore I expect you to upstream DTS of your product.

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 22, 2023, 8:45 a.m. UTC | #5 
On 22/11/2023 09:33, Jyan Chou [周芷安] wrote:
>>>> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
>>>> +      clock-names = "biu", "ciu", "vp0", "vp1";
>>>
>>> Where is the DTS implementing this binding?
>>>
>>>> I said it once:
>>>
>>>>> I asked you to test the bindings. This also means that you must test 
>>>>> your DTS against bindings. Your bindings, DTS and driver do not 
>>>>> match, therefore let's be a bit more clear:
>>>>>
>>>>> NAK, till you upstream your DTS.
>>>
>>>> I still do not see DTS anywhere. Please link it.
>>>
>>> Sorry for asking. Which dts do we need to link it? The device tree we use for our SOC?
>>>
>>> or realtek,rtd-dw-cqe-emmc.example.dts?  Thanks.
> 
>> The patch adding this device node to any upstream DTS. I am not speaking about example, but about upstream DTS using this MMC.
> 
>> As I wrote last time, I don't believe you test bindings and DTS and I had proofs of that. Therefore I expect you to upstream DTS of your product.
> 
> Sorry for explaining, since our DTS is still trying to upstream, but not accepted by upstream yet,

Please wrap your replies to match mailing list etiquette.

> 
> https://lore.kernel.org/linux-arm-kernel/4a462b2e-380a-9fd1-2e84-783cc457e8c2@linaro.org/T/

Last submission was half a year ago. That submission was not tested
either, so neither are your bindings nor DTS tested.

> 
> could I provide the dts we local used now for you first to compare with bindings? I am sorry for making this request.

I don't believe you are testing these, so the answer is not. Please send
your SoC for upstreaming with MMC nodes included or as follow up.

Best regards,
Krzysztof