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 <[email protected]>
Signed-off-by: Philipp Tomsich <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Philipp Tomsich <[email protected]>
---
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
This patch documents the new property disable-cqe-dcmd
for the Arasan eMMC 5.1 driver.
Signed-off-by: Christoph Muellner <[email protected]>
Signed-off-by: Philipp Tomsich <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Philipp Tomsich <[email protected]>
> ---
> 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?
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 <[email protected]>
> Signed-off-by: Philipp Tomsich <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
> ---
> 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);
>
> On 08.03.2019, at 13:46, Adrian Hunter <[email protected]> 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 <[email protected]>
>> Signed-off-by: Philipp Tomsich <[email protected]>
>> ---
>> 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?
Am Freitag, 8. M?rz 2019, 14:10:45 CET schrieb Christoph M?llner:
>
> > On 08.03.2019, at 13:46, Adrian Hunter <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Philipp Tomsich <[email protected]>
> >> ---
> >> 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/
> On 12.03.2019, at 14:17, Heiko Stuebner <[email protected]> wrote:
>
> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>>
>>> On 08.03.2019, at 13:46, Adrian Hunter <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Philipp Tomsich <[email protected]>
>>>> ---
>>>> 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
> On 12.03.2019, at 18:10, Heiko Stübner <[email protected]> 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 <[email protected]> wrote:
>>>
>>> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner:
>>>>> On 08.03.2019, at 13:46, Adrian Hunter <[email protected]> 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
>>>>>> <[email protected]> Signed-off-by: Philipp
>>>>>> Tomsich <[email protected]> ---
>>>>>> 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
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 <[email protected]> wrote:
> >
> > Am Freitag, 8. M?rz 2019, 14:10:45 CET schrieb Christoph M?llner:
> >>> On 08.03.2019, at 13:46, Adrian Hunter <[email protected]> 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
> >>>> <[email protected]> Signed-off-by: Philipp
> >>>> Tomsich <[email protected]> ---
> >>>> 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
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 <[email protected]>
> Signed-off-by: Philipp Tomsich <[email protected]>
> ---
> 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?