[PATCH v2 1/3] mmc: sdhci-of-arasan: Add DTS property to disable DCMDs.

[PATCH v2 1/3] mmc: sdhci-of-arasan: Add DTS property to disable DCMDs.

[Christoph Muellner]

Direct commands (DCMDs) are an optional feature of eMMC 5.1's command
queue engine (CQE). The Arasan eMMC 5.1 controller uses the CQHCI,
which exposes a control register bit to enable the feature.
The current implementation sets this bit unconditionally.

This patch allows to suppress the feature activation,
by specifying the property disable-cqe-dcmd.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index c9e3e050ccc8..88dc3f00a5be 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -832,7 +832,10 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		host->mmc_host_ops.start_signal_voltage_switch =
 					sdhci_arasan_voltage_switch;
 		sdhci_arasan->has_cqe = true;
-		host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
+		host->mmc->caps2 |= MMC_CAP2_CQE;
+
+		if (!of_property_read_bool(np, "disable-cqe-dcmd"))
+			host->mmc->caps2 |= MMC_CAP2_CQE_DCMD;
 	}
 
 	ret = sdhci_arasan_add_host(sdhci_arasan);
-- 
2.11.0

[PATCH v2 1/3] mmc: sdhci-of-arasan: Add DTS property to disable DCMDs.

[Christoph Muellner]

Direct commands (DCMDs) are an optional feature of eMMC 5.1's command
queue engine (CQE). The Arasan eMMC 5.1 controller uses the CQHCI,
which exposes a control register bit to enable the feature.
The current implementation sets this bit unconditionally.

This patch allows to suppress the feature activation,
by specifying the property disable-cqe-dcmd.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index c9e3e050ccc8..88dc3f00a5be 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -832,7 +832,10 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		host->mmc_host_ops.start_signal_voltage_switch =
 					sdhci_arasan_voltage_switch;
 		sdhci_arasan->has_cqe = true;
-		host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
+		host->mmc->caps2 |= MMC_CAP2_CQE;
+
+		if (!of_property_read_bool(np, "disable-cqe-dcmd"))
+			host->mmc->caps2 |= MMC_CAP2_CQE_DCMD;
 	}
 
 	ret = sdhci_arasan_add_host(sdhci_arasan);
-- 
2.11.0


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

[PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Muellner]

This patch documents the new property disable-cqe-dcmd
for the Arasan eMMC 5.1 driver.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 1edbb049cccb..ec699bf98b7c 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -44,6 +44,10 @@ Optional Properties:
     properly. Test mode can be used to force the controller to function.
   - xlnx,int-clock-stable-broken: when present, the controller always reports
     that the internal clock is stable even when it is not.
+  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
+    as part of the command queue engine (CQE). On controllers with a CQHCI,
+    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
+    This is done unless disable-cqe-dcmd is specified.
 
 Example:
 	sdhci@e0100000 {
-- 
2.11.0

[PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Muellner]

This patch documents the new property disable-cqe-dcmd
for the Arasan eMMC 5.1 driver.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 1edbb049cccb..ec699bf98b7c 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -44,6 +44,10 @@ Optional Properties:
     properly. Test mode can be used to force the controller to function.
   - xlnx,int-clock-stable-broken: when present, the controller always reports
     that the internal clock is stable even when it is not.
+  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
+    as part of the command queue engine (CQE). On controllers with a CQHCI,
+    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
+    This is done unless disable-cqe-dcmd is specified.
 
 Example:
 	sdhci@e0100000 {
-- 
2.11.0


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

[PATCH v2 3/3] arm64: dts: rockchip: Disable DCMDs on RK3399's eMMC controller.

[Christoph Muellner]

When using direct commands (DCMDs) on an RK3399, we get spurious
CQE completion interrupts for the DCMD transaction slot (#31):

[  931.196520] ------------[ cut here ]------------
[  931.201702] mmc1: cqhci: spurious TCN for tag 31
[  931.206906] WARNING: CPU: 0 PID: 1433 at
/usr/src/kernel/drivers/mmc/host/cqhci.c:725 cqhci_irq+0x2e4/0x490
[  931.206909] Modules linked in:
[  931.206918] CPU: 0 PID: 1433 Comm: irq/29-mmc1 Not tainted
4.19.8-rt6-funkadelic #1
[  931.206920] Hardware name: Theobroma Systems RK3399-Q7 SoM (DT)
[  931.206924] pstate: 40000005 (nZcv daif -PAN -UAO)
[  931.206927] pc : cqhci_irq+0x2e4/0x490
[  931.206931] lr : cqhci_irq+0x2e4/0x490
[  931.206933] sp : ffff00000e54bc80
[  931.206934] x29: ffff00000e54bc80 x28: 0000000000000000
[  931.206939] x27: 0000000000000001 x26: ffff000008f217e8
[  931.206944] x25: ffff8000f02ef030 x24: ffff0000091417b0
[  931.206948] x23: ffff0000090aa000 x22: ffff8000f008b000
[  931.206953] x21: 0000000000000002 x20: 000000000000001f
[  931.206957] x19: ffff8000f02ef018 x18: ffffffffffffffff
[  931.206961] x17: 0000000000000000 x16: 0000000000000000
[  931.206966] x15: ffff0000090aa6c8 x14: 0720072007200720
[  931.206970] x13: 0720072007200720 x12: 0720072007200720
[  931.206975] x11: 0720072007200720 x10: 0720072007200720
[  931.206980] x9 : 0720072007200720 x8 : 0720072007200720
[  931.206984] x7 : 0720073107330720 x6 : 00000000000005a0
[  931.206988] x5 : ffff00000860d4b0 x4 : 0000000000000000
[  931.206993] x3 : 0000000000000001 x2 : 0000000000000001
[  931.206997] x1 : 1bde3a91b0d4d900 x0 : 0000000000000000
[  931.207001] Call trace:
[  931.207005]  cqhci_irq+0x2e4/0x490
[  931.207009]  sdhci_arasan_cqhci_irq+0x5c/0x90
[  931.207013]  sdhci_irq+0x98/0x930
[  931.207019]  irq_forced_thread_fn+0x2c/0xa0
[  931.207023]  irq_thread+0x114/0x1c0
[  931.207027]  kthread+0x128/0x130
[  931.207032]  ret_from_fork+0x10/0x20
[  931.207035] ---[ end trace 0000000000000002 ]---

The driver shows this message only for the first spurious interrupt
by using WARN_ONCE(). Changing this to WARN() shows, that this is
happening quite frequently (up to once a second).

Since the eMMC 5.1 specification, where CQE and CQHCI are specified,
does not mention that spurious TCN interrupts for DCMDs can be simply
ignored, we must assume that using this feature is not working reliably.

The current implementation uses DCMD for REQ_OP_FLUSH only, and
I could not see any performance/power impact when disabling
this optional feature for RK3399.

Therefore this patch disables DCMDs for RK3399.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 6cc1c9fa4ea6..1bbf0da4e01d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -333,6 +333,7 @@
 		phys = <&emmc_phy>;
 		phy-names = "phy_arasan";
 		power-domains = <&power RK3399_PD_EMMC>;
+		disable-cqe-dcmd;
 		status = "disabled";
 	};
 
-- 
2.11.0

[PATCH v2 3/3] arm64: dts: rockchip: Disable DCMDs on RK3399's eMMC controller.

[Christoph Muellner]

When using direct commands (DCMDs) on an RK3399, we get spurious
CQE completion interrupts for the DCMD transaction slot (#31):

[  931.196520] ------------[ cut here ]------------
[  931.201702] mmc1: cqhci: spurious TCN for tag 31
[  931.206906] WARNING: CPU: 0 PID: 1433 at
/usr/src/kernel/drivers/mmc/host/cqhci.c:725 cqhci_irq+0x2e4/0x490
[  931.206909] Modules linked in:
[  931.206918] CPU: 0 PID: 1433 Comm: irq/29-mmc1 Not tainted
4.19.8-rt6-funkadelic #1
[  931.206920] Hardware name: Theobroma Systems RK3399-Q7 SoM (DT)
[  931.206924] pstate: 40000005 (nZcv daif -PAN -UAO)
[  931.206927] pc : cqhci_irq+0x2e4/0x490
[  931.206931] lr : cqhci_irq+0x2e4/0x490
[  931.206933] sp : ffff00000e54bc80
[  931.206934] x29: ffff00000e54bc80 x28: 0000000000000000
[  931.206939] x27: 0000000000000001 x26: ffff000008f217e8
[  931.206944] x25: ffff8000f02ef030 x24: ffff0000091417b0
[  931.206948] x23: ffff0000090aa000 x22: ffff8000f008b000
[  931.206953] x21: 0000000000000002 x20: 000000000000001f
[  931.206957] x19: ffff8000f02ef018 x18: ffffffffffffffff
[  931.206961] x17: 0000000000000000 x16: 0000000000000000
[  931.206966] x15: ffff0000090aa6c8 x14: 0720072007200720
[  931.206970] x13: 0720072007200720 x12: 0720072007200720
[  931.206975] x11: 0720072007200720 x10: 0720072007200720
[  931.206980] x9 : 0720072007200720 x8 : 0720072007200720
[  931.206984] x7 : 0720073107330720 x6 : 00000000000005a0
[  931.206988] x5 : ffff00000860d4b0 x4 : 0000000000000000
[  931.206993] x3 : 0000000000000001 x2 : 0000000000000001
[  931.206997] x1 : 1bde3a91b0d4d900 x0 : 0000000000000000
[  931.207001] Call trace:
[  931.207005]  cqhci_irq+0x2e4/0x490
[  931.207009]  sdhci_arasan_cqhci_irq+0x5c/0x90
[  931.207013]  sdhci_irq+0x98/0x930
[  931.207019]  irq_forced_thread_fn+0x2c/0xa0
[  931.207023]  irq_thread+0x114/0x1c0
[  931.207027]  kthread+0x128/0x130
[  931.207032]  ret_from_fork+0x10/0x20
[  931.207035] ---[ end trace 0000000000000002 ]---

The driver shows this message only for the first spurious interrupt
by using WARN_ONCE(). Changing this to WARN() shows, that this is
happening quite frequently (up to once a second).

Since the eMMC 5.1 specification, where CQE and CQHCI are specified,
does not mention that spurious TCN interrupts for DCMDs can be simply
ignored, we must assume that using this feature is not working reliably.

The current implementation uses DCMD for REQ_OP_FLUSH only, and
I could not see any performance/power impact when disabling
this optional feature for RK3399.

Therefore this patch disables DCMDs for RK3399.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 6cc1c9fa4ea6..1bbf0da4e01d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -333,6 +333,7 @@
 		phys = <&emmc_phy>;
 		phy-names = "phy_arasan";
 		power-domains = <&power RK3399_PD_EMMC>;
+		disable-cqe-dcmd;
 		status = "disabled";
 	};
 
-- 
2.11.0


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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Adrian Hunter]

On 7/03/19 10:43 AM, Christoph Muellner wrote:
> This patch documents the new property disable-cqe-dcmd
> for the Arasan eMMC 5.1 driver.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 1edbb049cccb..ec699bf98b7c 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -44,6 +44,10 @@ Optional Properties:
>      properly. Test mode can be used to force the controller to function.
>    - xlnx,int-clock-stable-broken: when present, the controller always reports
>      that the internal clock is stable even when it is not.
> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> +    This is done unless disable-cqe-dcmd is specified.

If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Adrian Hunter]

On 7/03/19 10:43 AM, Christoph Muellner wrote:
> This patch documents the new property disable-cqe-dcmd
> for the Arasan eMMC 5.1 driver.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 1edbb049cccb..ec699bf98b7c 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -44,6 +44,10 @@ Optional Properties:
>      properly. Test mode can be used to force the controller to function.
>    - xlnx,int-clock-stable-broken: when present, the controller always reports
>      that the internal clock is stable even when it is not.
> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> +    This is done unless disable-cqe-dcmd is specified.

If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?


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

Re: [PATCH v2 1/3] mmc: sdhci-of-arasan: Add DTS property to disable DCMDs.

[Adrian Hunter]

On 7/03/19 10:43 AM, Christoph Muellner wrote:
> Direct commands (DCMDs) are an optional feature of eMMC 5.1's command
> queue engine (CQE). The Arasan eMMC 5.1 controller uses the CQHCI,
> which exposes a control register bit to enable the feature.
> The current implementation sets this bit unconditionally.
> 
> This patch allows to suppress the feature activation,
> by specifying the property disable-cqe-dcmd.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index c9e3e050ccc8..88dc3f00a5be 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -832,7 +832,10 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  		host->mmc_host_ops.start_signal_voltage_switch =
>  					sdhci_arasan_voltage_switch;
>  		sdhci_arasan->has_cqe = true;
> -		host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
> +		host->mmc->caps2 |= MMC_CAP2_CQE;
> +
> +		if (!of_property_read_bool(np, "disable-cqe-dcmd"))
> +			host->mmc->caps2 |= MMC_CAP2_CQE_DCMD;
>  	}
>  
>  	ret = sdhci_arasan_add_host(sdhci_arasan);
> 

Re: [PATCH v2 1/3] mmc: sdhci-of-arasan: Add DTS property to disable DCMDs.

[Adrian Hunter]

On 7/03/19 10:43 AM, Christoph Muellner wrote:
> Direct commands (DCMDs) are an optional feature of eMMC 5.1's command
> queue engine (CQE). The Arasan eMMC 5.1 controller uses the CQHCI,
> which exposes a control register bit to enable the feature.
> The current implementation sets this bit unconditionally.
> 
> This patch allows to suppress the feature activation,
> by specifying the property disable-cqe-dcmd.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index c9e3e050ccc8..88dc3f00a5be 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -832,7 +832,10 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  		host->mmc_host_ops.start_signal_voltage_switch =
>  					sdhci_arasan_voltage_switch;
>  		sdhci_arasan->has_cqe = true;
> -		host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
> +		host->mmc->caps2 |= MMC_CAP2_CQE;
> +
> +		if (!of_property_read_bool(np, "disable-cqe-dcmd"))
> +			host->mmc->caps2 |= MMC_CAP2_CQE_DCMD;
>  	}
>  
>  	ret = sdhci_arasan_add_host(sdhci_arasan);
> 


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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>> This patch documents the new property disable-cqe-dcmd
>> for the Arasan eMMC 5.1 driver.
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index 1edbb049cccb..ec699bf98b7c 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -44,6 +44,10 @@ Optional Properties:
>>     properly. Test mode can be used to force the controller to function.
>>   - xlnx,int-clock-stable-broken: when present, the controller always reports
>>     that the internal clock is stable even when it is not.
>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
>> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
>> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
>> +    This is done unless disable-cqe-dcmd is specified.
> 
> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?

The file mmc.txt says on top:
"These properties are common to multiple MMC host controllers".
As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
I would say it should not go into that file.

Also I wonder why "supports-cqe" is in mmc.txt, because
only sdhci-tegra.c is evaluating that property.
So I would expect it to be documented in
Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt

However, I see that "disable-cqe-dcmd" could go into other drivers as well.
But is this enough to document it in mmc.txt?

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>> This patch documents the new property disable-cqe-dcmd
>> for the Arasan eMMC 5.1 driver.
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index 1edbb049cccb..ec699bf98b7c 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -44,6 +44,10 @@ Optional Properties:
>>     properly. Test mode can be used to force the controller to function.
>>   - xlnx,int-clock-stable-broken: when present, the controller always reports
>>     that the internal clock is stable even when it is not.
>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
>> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
>> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
>> +    This is done unless disable-cqe-dcmd is specified.
> 
> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?

The file mmc.txt says on top:
"These properties are common to multiple MMC host controllers".
As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
I would say it should not go into that file.

Also I wonder why "supports-cqe" is in mmc.txt, because
only sdhci-tegra.c is evaluating that property.
So I would expect it to be documented in
Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt

However, I see that "disable-cqe-dcmd" could go into other drivers as well.
But is this enough to document it in mmc.txt?

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>> This patch documents the new property disable-cqe-dcmd
>> for the Arasan eMMC 5.1 driver.
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index 1edbb049cccb..ec699bf98b7c 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -44,6 +44,10 @@ Optional Properties:
>>     properly. Test mode can be used to force the controller to function.
>>   - xlnx,int-clock-stable-broken: when present, the controller always reports
>>     that the internal clock is stable even when it is not.
>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
>> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
>> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
>> +    This is done unless disable-cqe-dcmd is specified.
> 
> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?

The file mmc.txt says on top:
"These properties are common to multiple MMC host controllers".
As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
I would say it should not go into that file.

Also I wonder why "supports-cqe" is in mmc.txt, because
only sdhci-tegra.c is evaluating that property.
So I would expect it to be documented in
Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt

However, I see that "disable-cqe-dcmd" could go into other drivers as well.
But is this enough to document it in mmc.txt?



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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Heiko Stuebner]

Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
> 
> > On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> > On 7/03/19 10:43 AM, Christoph Muellner wrote:
> >> This patch documents the new property disable-cqe-dcmd
> >> for the Arasan eMMC 5.1 driver.
> >> 
> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >> ---
> >> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> index 1edbb049cccb..ec699bf98b7c 100644
> >> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> @@ -44,6 +44,10 @@ Optional Properties:
> >>     properly. Test mode can be used to force the controller to function.
> >>   - xlnx,int-clock-stable-broken: when present, the controller always reports
> >>     that the internal clock is stable even when it is not.
> >> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> >> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> >> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> >> +    This is done unless disable-cqe-dcmd is specified.

This needs a rewording please. See below for hw-description vs. driver, so
the description should be centered around why this is a property of the hw
[like faulty controller implementation or whatever]


> > If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
> 
> The file mmc.txt says on top:
> "These properties are common to multiple MMC host controllers".
> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
> I would say it should not go into that file.
> 
> Also I wonder why "supports-cqe" is in mmc.txt, because
> only sdhci-tegra.c is evaluating that property.
> So I would expect it to be documented in
> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> 
> However, I see that "disable-cqe-dcmd" could go into other drivers as well.
> But is this enough to document it in mmc.txt?

Devicetree is always about describing the hardware capabilites and never
about the actual nitty-gritty of driver implementation, aka it is not meant
as a space for hardware-independent config-settings or such.

As for only tegra evaluating this, is probably because it is still so new, like
january 2019 and Rob explicitly suggested it becoming common [0], which
suggests that the disable-cqe-dcmds should probably also be common.

Heiko

[0] https://lore.kernel.org/patchwork/patch/1031163/

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Heiko Stuebner]

Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
> 
> > On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> > On 7/03/19 10:43 AM, Christoph Muellner wrote:
> >> This patch documents the new property disable-cqe-dcmd
> >> for the Arasan eMMC 5.1 driver.
> >> 
> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >> ---
> >> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> index 1edbb049cccb..ec699bf98b7c 100644
> >> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> @@ -44,6 +44,10 @@ Optional Properties:
> >>     properly. Test mode can be used to force the controller to function.
> >>   - xlnx,int-clock-stable-broken: when present, the controller always reports
> >>     that the internal clock is stable even when it is not.
> >> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> >> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> >> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> >> +    This is done unless disable-cqe-dcmd is specified.

This needs a rewording please. See below for hw-description vs. driver, so
the description should be centered around why this is a property of the hw
[like faulty controller implementation or whatever]


> > If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
> 
> The file mmc.txt says on top:
> "These properties are common to multiple MMC host controllers".
> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
> I would say it should not go into that file.
> 
> Also I wonder why "supports-cqe" is in mmc.txt, because
> only sdhci-tegra.c is evaluating that property.
> So I would expect it to be documented in
> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> 
> However, I see that "disable-cqe-dcmd" could go into other drivers as well.
> But is this enough to document it in mmc.txt?

Devicetree is always about describing the hardware capabilites and never
about the actual nitty-gritty of driver implementation, aka it is not meant
as a space for hardware-independent config-settings or such.

As for only tegra evaluating this, is probably because it is still so new, like
january 2019 and Rob explicitly suggested it becoming common [0], which
suggests that the disable-cqe-dcmds should probably also be common.

Heiko

[0] https://lore.kernel.org/patchwork/patch/1031163/

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Heiko Stuebner]

Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
> 
> > On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> > On 7/03/19 10:43 AM, Christoph Muellner wrote:
> >> This patch documents the new property disable-cqe-dcmd
> >> for the Arasan eMMC 5.1 driver.
> >> 
> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >> ---
> >> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> index 1edbb049cccb..ec699bf98b7c 100644
> >> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >> @@ -44,6 +44,10 @@ Optional Properties:
> >>     properly. Test mode can be used to force the controller to function.
> >>   - xlnx,int-clock-stable-broken: when present, the controller always reports
> >>     that the internal clock is stable even when it is not.
> >> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> >> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> >> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> >> +    This is done unless disable-cqe-dcmd is specified.

This needs a rewording please. See below for hw-description vs. driver, so
the description should be centered around why this is a property of the hw
[like faulty controller implementation or whatever]


> > If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
> 
> The file mmc.txt says on top:
> "These properties are common to multiple MMC host controllers".
> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
> I would say it should not go into that file.
> 
> Also I wonder why "supports-cqe" is in mmc.txt, because
> only sdhci-tegra.c is evaluating that property.
> So I would expect it to be documented in
> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> 
> However, I see that "disable-cqe-dcmd" could go into other drivers as well.
> But is this enough to document it in mmc.txt?

Devicetree is always about describing the hardware capabilites and never
about the actual nitty-gritty of driver implementation, aka it is not meant
as a space for hardware-independent config-settings or such.

As for only tegra evaluating this, is probably because it is still so new, like
january 2019 and Rob explicitly suggested it becoming common [0], which
suggests that the disable-cqe-dcmds should probably also be common.

Heiko

[0] https://lore.kernel.org/patchwork/patch/1031163/



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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>> 
>>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> 
>>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>>>> This patch documents the new property disable-cqe-dcmd
>>>> for the Arasan eMMC 5.1 driver.
>>>> 
>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> ---
>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> index 1edbb049cccb..ec699bf98b7c 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> @@ -44,6 +44,10 @@ Optional Properties:
>>>>    properly. Test mode can be used to force the controller to function.
>>>>  - xlnx,int-clock-stable-broken: when present, the controller always reports
>>>>    that the internal clock is stable even when it is not.
>>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
>>>> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
>>>> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
>>>> +    This is done unless disable-cqe-dcmd is specified.
> 
> This needs a rewording please. See below for hw-description vs. driver, so
> the description should be centered around why this is a property of the hw
> [like faulty controller implementation or whatever]

I understand, that you prefer a HW-specific name for a property
over one, that explains the actual effect.
However, using disable-<feature> (which is always a directive for the driver
and not a HW description) is not uncommon:

* disable-wp should be "no-wp-line-connection"
* disable-over-current -> "no-over-current-line-connection"
* srp-disable -> "not-existing-srp-implementation"
* ...

But I don't mind using something else.
Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
Or other suggestions?

Also I'd like to mention, that my first implementation was "supports-cqe-dcmd".
I guess that would be a good choice, if it would have been there
from the beginning. Introducing it now, would silently disable DCMD
for existing DTBs. Therefore I went on to "disable-cqe-dcmd".

> 
> 
>>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
>> 
>> The file mmc.txt says on top:
>> "These properties are common to multiple MMC host controllers".
>> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
>> I would say it should not go into that file.
>> 
>> Also I wonder why "supports-cqe" is in mmc.txt, because
>> only sdhci-tegra.c is evaluating that property.
>> So I would expect it to be documented in
>> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
>> 
>> However, I see that "disable-cqe-dcmd" could go into other drivers as well.
>> But is this enough to document it in mmc.txt?
> 
> Devicetree is always about describing the hardware capabilites and never
> about the actual nitty-gritty of driver implementation, aka it is not meant
> as a space for hardware-independent config-settings or such.
> 
> As for only tegra evaluating this, is probably because it is still so new, like
> january 2019 and Rob explicitly suggested it becoming common [0], which
> suggests that the disable-cqe-dcmds should probably also be common.

So mmc.txt lists "standardised" names for properties to reduce the risk
of having similar, but distinct names for different MMC drivers.
So whenever you want to introduce a new property for a driver,
check if there isn't already something defined in mmc.txt.

When seeing it this way, it clearly makes sense to have the property there.

Thanks,
Christoph

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>> 
>>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> 
>>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>>>> This patch documents the new property disable-cqe-dcmd
>>>> for the Arasan eMMC 5.1 driver.
>>>> 
>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> ---
>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> index 1edbb049cccb..ec699bf98b7c 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> @@ -44,6 +44,10 @@ Optional Properties:
>>>>    properly. Test mode can be used to force the controller to function.
>>>>  - xlnx,int-clock-stable-broken: when present, the controller always reports
>>>>    that the internal clock is stable even when it is not.
>>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
>>>> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
>>>> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
>>>> +    This is done unless disable-cqe-dcmd is specified.
> 
> This needs a rewording please. See below for hw-description vs. driver, so
> the description should be centered around why this is a property of the hw
> [like faulty controller implementation or whatever]

I understand, that you prefer a HW-specific name for a property
over one, that explains the actual effect.
However, using disable-<feature> (which is always a directive for the driver
and not a HW description) is not uncommon:

* disable-wp should be "no-wp-line-connection"
* disable-over-current -> "no-over-current-line-connection"
* srp-disable -> "not-existing-srp-implementation"
* ...

But I don't mind using something else.
Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
Or other suggestions?

Also I'd like to mention, that my first implementation was "supports-cqe-dcmd".
I guess that would be a good choice, if it would have been there
from the beginning. Introducing it now, would silently disable DCMD
for existing DTBs. Therefore I went on to "disable-cqe-dcmd".

> 
> 
>>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
>> 
>> The file mmc.txt says on top:
>> "These properties are common to multiple MMC host controllers".
>> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
>> I would say it should not go into that file.
>> 
>> Also I wonder why "supports-cqe" is in mmc.txt, because
>> only sdhci-tegra.c is evaluating that property.
>> So I would expect it to be documented in
>> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
>> 
>> However, I see that "disable-cqe-dcmd" could go into other drivers as well.
>> But is this enough to document it in mmc.txt?
> 
> Devicetree is always about describing the hardware capabilites and never
> about the actual nitty-gritty of driver implementation, aka it is not meant
> as a space for hardware-independent config-settings or such.
> 
> As for only tegra evaluating this, is probably because it is still so new, like
> january 2019 and Rob explicitly suggested it becoming common [0], which
> suggests that the disable-cqe-dcmds should probably also be common.

So mmc.txt lists "standardised" names for properties to reduce the risk
of having similar, but distinct names for different MMC drivers.
So whenever you want to introduce a new property for a driver,
check if there isn't already something defined in mmc.txt.

When seeing it this way, it clearly makes sense to have the property there.

Thanks,
Christoph

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>> 
>>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> 
>>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>>>> This patch documents the new property disable-cqe-dcmd
>>>> for the Arasan eMMC 5.1 driver.
>>>> 
>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> ---
>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> index 1edbb049cccb..ec699bf98b7c 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>> @@ -44,6 +44,10 @@ Optional Properties:
>>>>    properly. Test mode can be used to force the controller to function.
>>>>  - xlnx,int-clock-stable-broken: when present, the controller always reports
>>>>    that the internal clock is stable even when it is not.
>>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
>>>> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
>>>> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
>>>> +    This is done unless disable-cqe-dcmd is specified.
> 
> This needs a rewording please. See below for hw-description vs. driver, so
> the description should be centered around why this is a property of the hw
> [like faulty controller implementation or whatever]

I understand, that you prefer a HW-specific name for a property
over one, that explains the actual effect.
However, using disable-<feature> (which is always a directive for the driver
and not a HW description) is not uncommon:

* disable-wp should be "no-wp-line-connection"
* disable-over-current -> "no-over-current-line-connection"
* srp-disable -> "not-existing-srp-implementation"
* ...

But I don't mind using something else.
Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
Or other suggestions?

Also I'd like to mention, that my first implementation was "supports-cqe-dcmd".
I guess that would be a good choice, if it would have been there
from the beginning. Introducing it now, would silently disable DCMD
for existing DTBs. Therefore I went on to "disable-cqe-dcmd".

> 
> 
>>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
>> 
>> The file mmc.txt says on top:
>> "These properties are common to multiple MMC host controllers".
>> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
>> I would say it should not go into that file.
>> 
>> Also I wonder why "supports-cqe" is in mmc.txt, because
>> only sdhci-tegra.c is evaluating that property.
>> So I would expect it to be documented in
>> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
>> 
>> However, I see that "disable-cqe-dcmd" could go into other drivers as well.
>> But is this enough to document it in mmc.txt?
> 
> Devicetree is always about describing the hardware capabilites and never
> about the actual nitty-gritty of driver implementation, aka it is not meant
> as a space for hardware-independent config-settings or such.
> 
> As for only tegra evaluating this, is probably because it is still so new, like
> january 2019 and Rob explicitly suggested it becoming common [0], which
> suggests that the disable-cqe-dcmds should probably also be common.

So mmc.txt lists "standardised" names for properties to reduce the risk
of having similar, but distinct names for different MMC drivers.
So whenever you want to introduce a new property for a driver,
check if there isn't already something defined in mmc.txt.

When seeing it this way, it clearly makes sense to have the property there.

Thanks,
Christoph


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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Heiko Stübner]

Hi Christoph,

Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner:
> > On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
> > 
> > Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
> >>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>> 
> >>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
> >>>> This patch documents the new property disable-cqe-dcmd
> >>>> for the Arasan eMMC 5.1 driver.
> >>>> 
> >>>> Signed-off-by: Christoph Muellner
> >>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp
> >>>> Tomsich <philipp.tomsich@theobroma-systems.com> ---
> >>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index
> >>>> 1edbb049cccb..ec699bf98b7c 100644
> >>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> 
> >>>> @@ -44,6 +44,10 @@ Optional Properties:
> >>>>    properly. Test mode can be used to force the controller to function.
> >>>>  
> >>>>  - xlnx,int-clock-stable-broken: when present, the controller always
> >>>>  reports
> >>>>  
> >>>>    that the internal clock is stable even when it is not.
> >>>> 
> >>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands
> >>>> (DCMDs) +    as part of the command queue engine (CQE). On controllers
> >>>> with a CQHCI, +    such as the Arasan eMMC 5.1 host controller, the
> >>>> driver has to enable DCMDs. +    This is done unless disable-cqe-dcmd
> >>>> is specified.
> > 
> > This needs a rewording please. See below for hw-description vs. driver, so
> > the description should be centered around why this is a property of the hw
> > [like faulty controller implementation or whatever]
> 
> I understand, that you prefer a HW-specific name for a property
> over one, that explains the actual effect.

Actually I think the property-name is just fine :-) .

The description might profit from a rewording though. Aka not driver-
centric but hw-centric, describing why some controllers may need the
option to disable these dcmds (controller bugs or whatever).


Heiko



> However, using disable-<feature> (which is always a directive for the driver
> and not a HW description) is not uncommon:
> 
> * disable-wp should be "no-wp-line-connection"
> * disable-over-current -> "no-over-current-line-connection"
> * srp-disable -> "not-existing-srp-implementation"
> * ...
> 
> But I don't mind using something else.
> Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
> Or other suggestions?
> 
> Also I'd like to mention, that my first implementation was
> "supports-cqe-dcmd". I guess that would be a good choice, if it would have
> been there
> from the beginning. Introducing it now, would silently disable DCMD
> for existing DTBs. Therefore I went on to "disable-cqe-dcmd".
> 
> >>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there
> >>> also?
> >> 
> >> The file mmc.txt says on top:
> >> "These properties are common to multiple MMC host controllers".
> >> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
> >> I would say it should not go into that file.
> >> 
> >> Also I wonder why "supports-cqe" is in mmc.txt, because
> >> only sdhci-tegra.c is evaluating that property.
> >> So I would expect it to be documented in
> >> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> >> 
> >> However, I see that "disable-cqe-dcmd" could go into other drivers as
> >> well.
> >> But is this enough to document it in mmc.txt?
> > 
> > Devicetree is always about describing the hardware capabilites and never
> > about the actual nitty-gritty of driver implementation, aka it is not
> > meant
> > as a space for hardware-independent config-settings or such.
> > 
> > As for only tegra evaluating this, is probably because it is still so new,
> > like january 2019 and Rob explicitly suggested it becoming common [0],
> > which suggests that the disable-cqe-dcmds should probably also be common.
> So mmc.txt lists "standardised" names for properties to reduce the risk
> of having similar, but distinct names for different MMC drivers.
> So whenever you want to introduce a new property for a driver,
> check if there isn't already something defined in mmc.txt.
> 
> When seeing it this way, it clearly makes sense to have the property there.
> 
> Thanks,
> Christoph

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Heiko Stübner]

Hi Christoph,

Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner:
> > On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
> > 
> > Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
> >>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>> 
> >>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
> >>>> This patch documents the new property disable-cqe-dcmd
> >>>> for the Arasan eMMC 5.1 driver.
> >>>> 
> >>>> Signed-off-by: Christoph Muellner
> >>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp
> >>>> Tomsich <philipp.tomsich@theobroma-systems.com> ---
> >>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index
> >>>> 1edbb049cccb..ec699bf98b7c 100644
> >>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> 
> >>>> @@ -44,6 +44,10 @@ Optional Properties:
> >>>>    properly. Test mode can be used to force the controller to function.
> >>>>  
> >>>>  - xlnx,int-clock-stable-broken: when present, the controller always
> >>>>  reports
> >>>>  
> >>>>    that the internal clock is stable even when it is not.
> >>>> 
> >>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands
> >>>> (DCMDs) +    as part of the command queue engine (CQE). On controllers
> >>>> with a CQHCI, +    such as the Arasan eMMC 5.1 host controller, the
> >>>> driver has to enable DCMDs. +    This is done unless disable-cqe-dcmd
> >>>> is specified.
> > 
> > This needs a rewording please. See below for hw-description vs. driver, so
> > the description should be centered around why this is a property of the hw
> > [like faulty controller implementation or whatever]
> 
> I understand, that you prefer a HW-specific name for a property
> over one, that explains the actual effect.

Actually I think the property-name is just fine :-) .

The description might profit from a rewording though. Aka not driver-
centric but hw-centric, describing why some controllers may need the
option to disable these dcmds (controller bugs or whatever).


Heiko



> However, using disable-<feature> (which is always a directive for the driver
> and not a HW description) is not uncommon:
> 
> * disable-wp should be "no-wp-line-connection"
> * disable-over-current -> "no-over-current-line-connection"
> * srp-disable -> "not-existing-srp-implementation"
> * ...
> 
> But I don't mind using something else.
> Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
> Or other suggestions?
> 
> Also I'd like to mention, that my first implementation was
> "supports-cqe-dcmd". I guess that would be a good choice, if it would have
> been there
> from the beginning. Introducing it now, would silently disable DCMD
> for existing DTBs. Therefore I went on to "disable-cqe-dcmd".
> 
> >>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there
> >>> also?
> >> 
> >> The file mmc.txt says on top:
> >> "These properties are common to multiple MMC host controllers".
> >> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
> >> I would say it should not go into that file.
> >> 
> >> Also I wonder why "supports-cqe" is in mmc.txt, because
> >> only sdhci-tegra.c is evaluating that property.
> >> So I would expect it to be documented in
> >> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> >> 
> >> However, I see that "disable-cqe-dcmd" could go into other drivers as
> >> well.
> >> But is this enough to document it in mmc.txt?
> > 
> > Devicetree is always about describing the hardware capabilites and never
> > about the actual nitty-gritty of driver implementation, aka it is not
> > meant
> > as a space for hardware-independent config-settings or such.
> > 
> > As for only tegra evaluating this, is probably because it is still so new,
> > like january 2019 and Rob explicitly suggested it becoming common [0],
> > which suggests that the disable-cqe-dcmds should probably also be common.
> So mmc.txt lists "standardised" names for properties to reduce the risk
> of having similar, but distinct names for different MMC drivers.
> So whenever you want to introduce a new property for a driver,
> check if there isn't already something defined in mmc.txt.
> 
> When seeing it this way, it clearly makes sense to have the property there.
> 
> Thanks,
> Christoph

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Heiko Stübner]

Hi Christoph,

Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner:
> > On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
> > 
> > Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
> >>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>> 
> >>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
> >>>> This patch documents the new property disable-cqe-dcmd
> >>>> for the Arasan eMMC 5.1 driver.
> >>>> 
> >>>> Signed-off-by: Christoph Muellner
> >>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp
> >>>> Tomsich <philipp.tomsich@theobroma-systems.com> ---
> >>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index
> >>>> 1edbb049cccb..ec699bf98b7c 100644
> >>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> >>>> 
> >>>> @@ -44,6 +44,10 @@ Optional Properties:
> >>>>    properly. Test mode can be used to force the controller to function.
> >>>>  
> >>>>  - xlnx,int-clock-stable-broken: when present, the controller always
> >>>>  reports
> >>>>  
> >>>>    that the internal clock is stable even when it is not.
> >>>> 
> >>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands
> >>>> (DCMDs) +    as part of the command queue engine (CQE). On controllers
> >>>> with a CQHCI, +    such as the Arasan eMMC 5.1 host controller, the
> >>>> driver has to enable DCMDs. +    This is done unless disable-cqe-dcmd
> >>>> is specified.
> > 
> > This needs a rewording please. See below for hw-description vs. driver, so
> > the description should be centered around why this is a property of the hw
> > [like faulty controller implementation or whatever]
> 
> I understand, that you prefer a HW-specific name for a property
> over one, that explains the actual effect.

Actually I think the property-name is just fine :-) .

The description might profit from a rewording though. Aka not driver-
centric but hw-centric, describing why some controllers may need the
option to disable these dcmds (controller bugs or whatever).


Heiko



> However, using disable-<feature> (which is always a directive for the driver
> and not a HW description) is not uncommon:
> 
> * disable-wp should be "no-wp-line-connection"
> * disable-over-current -> "no-over-current-line-connection"
> * srp-disable -> "not-existing-srp-implementation"
> * ...
> 
> But I don't mind using something else.
> Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
> Or other suggestions?
> 
> Also I'd like to mention, that my first implementation was
> "supports-cqe-dcmd". I guess that would be a good choice, if it would have
> been there
> from the beginning. Introducing it now, would silently disable DCMD
> for existing DTBs. Therefore I went on to "disable-cqe-dcmd".
> 
> >>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there
> >>> also?
> >> 
> >> The file mmc.txt says on top:
> >> "These properties are common to multiple MMC host controllers".
> >> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
> >> I would say it should not go into that file.
> >> 
> >> Also I wonder why "supports-cqe" is in mmc.txt, because
> >> only sdhci-tegra.c is evaluating that property.
> >> So I would expect it to be documented in
> >> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> >> 
> >> However, I see that "disable-cqe-dcmd" could go into other drivers as
> >> well.
> >> But is this enough to document it in mmc.txt?
> > 
> > Devicetree is always about describing the hardware capabilites and never
> > about the actual nitty-gritty of driver implementation, aka it is not
> > meant
> > as a space for hardware-independent config-settings or such.
> > 
> > As for only tegra evaluating this, is probably because it is still so new,
> > like january 2019 and Rob explicitly suggested it becoming common [0],
> > which suggests that the disable-cqe-dcmds should probably also be common.
> So mmc.txt lists "standardised" names for properties to reduce the risk
> of having similar, but distinct names for different MMC drivers.
> So whenever you want to introduce a new property for a driver,
> check if there isn't already something defined in mmc.txt.
> 
> When seeing it this way, it clearly makes sense to have the property there.
> 
> Thanks,
> Christoph





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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 12.03.2019, at 18:10, Heiko Stübner <heiko@sntech.de> wrote:
> 
> Hi Christoph,
> 
> Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner:
>>> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
>>> 
>>> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>>>>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> 
>>>>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>>>>>> This patch documents the new property disable-cqe-dcmd
>>>>>> for the Arasan eMMC 5.1 driver.
>>>>>> 
>>>>>> Signed-off-by: Christoph Muellner
>>>>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp
>>>>>> Tomsich <philipp.tomsich@theobroma-systems.com> ---
>>>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>> 
>>>>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index
>>>>>> 1edbb049cccb..ec699bf98b7c 100644
>>>>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>>>> 
>>>>>> @@ -44,6 +44,10 @@ Optional Properties:
>>>>>>   properly. Test mode can be used to force the controller to function.
>>>>>> 
>>>>>> - xlnx,int-clock-stable-broken: when present, the controller always
>>>>>> reports
>>>>>> 
>>>>>>   that the internal clock is stable even when it is not.
>>>>>> 
>>>>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands
>>>>>> (DCMDs) +    as part of the command queue engine (CQE). On controllers
>>>>>> with a CQHCI, +    such as the Arasan eMMC 5.1 host controller, the
>>>>>> driver has to enable DCMDs. +    This is done unless disable-cqe-dcmd
>>>>>> is specified.
>>> 
>>> This needs a rewording please. See below for hw-description vs. driver, so
>>> the description should be centered around why this is a property of the hw
>>> [like faulty controller implementation or whatever]
>> 
>> I understand, that you prefer a HW-specific name for a property
>> over one, that explains the actual effect.
> 
> Actually I think the property-name is just fine :-) .
> 
> The description might profit from a rewording though. Aka not driver-
> centric but hw-centric, describing why some controllers may need the
> option to disable these dcmds (controller bugs or whatever).

Ah, ok. Now I understand what you mean.
Will do for v3.

Thanks,
Christoph

> 
> 
> Heiko
> 
> 
> 
>> However, using disable-<feature> (which is always a directive for the driver
>> and not a HW description) is not uncommon:
>> 
>> * disable-wp should be "no-wp-line-connection"
>> * disable-over-current -> "no-over-current-line-connection"
>> * srp-disable -> "not-existing-srp-implementation"
>> * ...
>> 
>> But I don't mind using something else.
>> Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
>> Or other suggestions?
>> 
>> Also I'd like to mention, that my first implementation was
>> "supports-cqe-dcmd". I guess that would be a good choice, if it would have
>> been there
>> from the beginning. Introducing it now, would silently disable DCMD
>> for existing DTBs. Therefore I went on to "disable-cqe-dcmd".
>> 
>>>>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there
>>>>> also?
>>>> 
>>>> The file mmc.txt says on top:
>>>> "These properties are common to multiple MMC host controllers".
>>>> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
>>>> I would say it should not go into that file.
>>>> 
>>>> Also I wonder why "supports-cqe" is in mmc.txt, because
>>>> only sdhci-tegra.c is evaluating that property.
>>>> So I would expect it to be documented in
>>>> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
>>>> 
>>>> However, I see that "disable-cqe-dcmd" could go into other drivers as
>>>> well.
>>>> But is this enough to document it in mmc.txt?
>>> 
>>> Devicetree is always about describing the hardware capabilites and never
>>> about the actual nitty-gritty of driver implementation, aka it is not
>>> meant
>>> as a space for hardware-independent config-settings or such.
>>> 
>>> As for only tegra evaluating this, is probably because it is still so new,
>>> like january 2019 and Rob explicitly suggested it becoming common [0],
>>> which suggests that the disable-cqe-dcmds should probably also be common.
>> So mmc.txt lists "standardised" names for properties to reduce the risk
>> of having similar, but distinct names for different MMC drivers.
>> So whenever you want to introduce a new property for a driver,
>> check if there isn't already something defined in mmc.txt.
>> 
>> When seeing it this way, it clearly makes sense to have the property there.
>> 
>> Thanks,
>> Christoph

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Christoph Müllner]

> On 12.03.2019, at 18:10, Heiko Stübner <heiko@sntech.de> wrote:
> 
> Hi Christoph,
> 
> Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner:
>>> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote:
>>> 
>>> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>>>>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> 
>>>>> On 7/03/19 10:43 AM, Christoph Muellner wrote:
>>>>>> This patch documents the new property disable-cqe-dcmd
>>>>>> for the Arasan eMMC 5.1 driver.
>>>>>> 
>>>>>> Signed-off-by: Christoph Muellner
>>>>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp
>>>>>> Tomsich <philipp.tomsich@theobroma-systems.com> ---
>>>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>> 
>>>>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index
>>>>>> 1edbb049cccb..ec699bf98b7c 100644
>>>>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>>>>> 
>>>>>> @@ -44,6 +44,10 @@ Optional Properties:
>>>>>>   properly. Test mode can be used to force the controller to function.
>>>>>> 
>>>>>> - xlnx,int-clock-stable-broken: when present, the controller always
>>>>>> reports
>>>>>> 
>>>>>>   that the internal clock is stable even when it is not.
>>>>>> 
>>>>>> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands
>>>>>> (DCMDs) +    as part of the command queue engine (CQE). On controllers
>>>>>> with a CQHCI, +    such as the Arasan eMMC 5.1 host controller, the
>>>>>> driver has to enable DCMDs. +    This is done unless disable-cqe-dcmd
>>>>>> is specified.
>>> 
>>> This needs a rewording please. See below for hw-description vs. driver, so
>>> the description should be centered around why this is a property of the hw
>>> [like faulty controller implementation or whatever]
>> 
>> I understand, that you prefer a HW-specific name for a property
>> over one, that explains the actual effect.
> 
> Actually I think the property-name is just fine :-) .
> 
> The description might profit from a rewording though. Aka not driver-
> centric but hw-centric, describing why some controllers may need the
> option to disable these dcmds (controller bugs or whatever).

Ah, ok. Now I understand what you mean.
Will do for v3.

Thanks,
Christoph

> 
> 
> Heiko
> 
> 
> 
>> However, using disable-<feature> (which is always a directive for the driver
>> and not a HW description) is not uncommon:
>> 
>> * disable-wp should be "no-wp-line-connection"
>> * disable-over-current -> "no-over-current-line-connection"
>> * srp-disable -> "not-existing-srp-implementation"
>> * ...
>> 
>> But I don't mind using something else.
>> Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok?
>> Or other suggestions?
>> 
>> Also I'd like to mention, that my first implementation was
>> "supports-cqe-dcmd". I guess that would be a good choice, if it would have
>> been there
>> from the beginning. Introducing it now, would silently disable DCMD
>> for existing DTBs. Therefore I went on to "disable-cqe-dcmd".
>> 
>>>>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there
>>>>> also?
>>>> 
>>>> The file mmc.txt says on top:
>>>> "These properties are common to multiple MMC host controllers".
>>>> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan,
>>>> I would say it should not go into that file.
>>>> 
>>>> Also I wonder why "supports-cqe" is in mmc.txt, because
>>>> only sdhci-tegra.c is evaluating that property.
>>>> So I would expect it to be documented in
>>>> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
>>>> 
>>>> However, I see that "disable-cqe-dcmd" could go into other drivers as
>>>> well.
>>>> But is this enough to document it in mmc.txt?
>>> 
>>> Devicetree is always about describing the hardware capabilites and never
>>> about the actual nitty-gritty of driver implementation, aka it is not
>>> meant
>>> as a space for hardware-independent config-settings or such.
>>> 
>>> As for only tegra evaluating this, is probably because it is still so new,
>>> like january 2019 and Rob explicitly suggested it becoming common [0],
>>> which suggests that the disable-cqe-dcmds should probably also be common.
>> So mmc.txt lists "standardised" names for properties to reduce the risk
>> of having similar, but distinct names for different MMC drivers.
>> So whenever you want to introduce a new property for a driver,
>> check if there isn't already something defined in mmc.txt.
>> 
>> When seeing it this way, it clearly makes sense to have the property there.
>> 
>> Thanks,
>> Christoph


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

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Rob Herring]

On Thu, Mar 07, 2019 at 09:43:27AM +0100, Christoph Muellner wrote:
> This patch documents the new property disable-cqe-dcmd
> for the Arasan eMMC 5.1 driver.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 1edbb049cccb..ec699bf98b7c 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -44,6 +44,10 @@ Optional Properties:
>      properly. Test mode can be used to force the controller to function.
>    - xlnx,int-clock-stable-broken: when present, the controller always reports
>      that the internal clock is stable even when it is not.
> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> +    This is done unless disable-cqe-dcmd is specified.

Should be common either for all controllers or all SDHCI controllers?

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Rob Herring]

On Thu, Mar 07, 2019 at 09:43:27AM +0100, Christoph Muellner wrote:
> This patch documents the new property disable-cqe-dcmd
> for the Arasan eMMC 5.1 driver.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 1edbb049cccb..ec699bf98b7c 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -44,6 +44,10 @@ Optional Properties:
>      properly. Test mode can be used to force the controller to function.
>    - xlnx,int-clock-stable-broken: when present, the controller always reports
>      that the internal clock is stable even when it is not.
> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> +    This is done unless disable-cqe-dcmd is specified.

Should be common either for all controllers or all SDHCI controllers?

Re: [PATCH v2 2/3] dt-bindings: mmc: Add a new property disable-cqe-dcmd.

[Rob Herring]

On Thu, Mar 07, 2019 at 09:43:27AM +0100, Christoph Muellner wrote:
> This patch documents the new property disable-cqe-dcmd
> for the Arasan eMMC 5.1 driver.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 1edbb049cccb..ec699bf98b7c 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -44,6 +44,10 @@ Optional Properties:
>      properly. Test mode can be used to force the controller to function.
>    - xlnx,int-clock-stable-broken: when present, the controller always reports
>      that the internal clock is stable even when it is not.
> +  - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs)
> +    as part of the command queue engine (CQE). On controllers with a CQHCI,
> +    such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs.
> +    This is done unless disable-cqe-dcmd is specified.

Should be common either for all controllers or all SDHCI controllers?

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