| Message ID | 20190322113808.16858-1-christoph.muellner@theobroma-systems.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v3,1/3] dt-bindings: mmc: Add disable-cqe-dcmd property. | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Fri, 22 Mar 2019 at 12:39, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote: > > Add disable-cqe-dcmd as optional property for MMC hosts. > This property allows to disable or not enable the direct command > features of the command queue engine. > > Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com> > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> Applied for next, thanks! Kind regards Uffe > --- > Documentation/devicetree/bindings/mmc/mmc.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt > index cdbcfd3a4ff2..c269dbe384fe 100644 > --- a/Documentation/devicetree/bindings/mmc/mmc.txt > +++ b/Documentation/devicetree/bindings/mmc/mmc.txt > @@ -64,6 +64,8 @@ Optional properties: > whether pwrseq-simple is used. Default to 10ms if no available. > - supports-cqe : The presence of this property indicates that the corresponding > MMC host controller supports HW command queue feature. > +- disable-cqe-dcmd: This property indicates that the MMC controller's command > + queue engine (CQE) does not support direct commands (DCMDs). > > *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line > polarity properties, we have to fix the meaning of the "normal" and "inverted" > -- > 2.11.0 >
On Fri, 22 Mar 2019 at 12:39, Christoph Muellner <christoph.muellner@theobroma-systems.com> 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> Applied for next, thanks! Perhaps both patch1 and patch2 should be tagged for stable? Then please tell me and I can add a tag. Kind regards Uffe > --- > 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 >
On Fri, 22 Mar 2019 at 12:39, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote: > > 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> FYI, I leave this for arm-soc to pick up. Kind regards Uffe > --- > 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 db9d948c0b03..1a16d6ce3ea8 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 >
> On 25.03.2019, at 14:27, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 22 Mar 2019 at 12:39, Christoph Muellner > <christoph.muellner@theobroma-systems.com> 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> > > Applied for next, thanks! > > Perhaps both patch1 and patch2 should be tagged for stable? Then > please tell me and I can add a tag. Hi Ulf, yes tagging for stable would be nice. I guess, the following would fit: Fixes: 84362d79f436 ("mmc: sdhci-of-arasan: Add CQHCI support for arasan,sdhci-5.1") Thanks, Christoph > > Kind regards > Uffe > > > >> --- >> 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 >>
On Mon, 25 Mar 2019 at 14:39, Christoph Müllner <christoph.muellner@theobroma-systems.com> wrote: > > > > On 25.03.2019, at 14:27, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 22 Mar 2019 at 12:39, Christoph Muellner > > <christoph.muellner@theobroma-systems.com> 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> > > > > Applied for next, thanks! > > > > Perhaps both patch1 and patch2 should be tagged for stable? Then > > please tell me and I can add a tag. > > Hi Ulf, > > yes tagging for stable would be nice. > I guess, the following would fit: > > Fixes: 84362d79f436 ("mmc: sdhci-of-arasan: Add CQHCI support for arasan,sdhci-5.1") > Great, I have added that to both of the commits and a stable tag. Thanks! [...] Kind regards Uffe
Am Montag, 25. März 2019, 14:27:26 CET schrieb Ulf Hansson: > On Fri, 22 Mar 2019 at 12:39, Christoph Muellner > <christoph.muellner@theobroma-systems.com> wrote: > > > > 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> > > FYI, I leave this for arm-soc to pick up. I've duplicated your fixes+stable tags and applied for 5.2 Thanks Heiko
diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt index cdbcfd3a4ff2..c269dbe384fe 100644 --- a/Documentation/devicetree/bindings/mmc/mmc.txt +++ b/Documentation/devicetree/bindings/mmc/mmc.txt @@ -64,6 +64,8 @@ Optional properties: whether pwrseq-simple is used. Default to 10ms if no available. - supports-cqe : The presence of this property indicates that the corresponding MMC host controller supports HW command queue feature. +- disable-cqe-dcmd: This property indicates that the MMC controller's command + queue engine (CQE) does not support direct commands (DCMDs). *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line polarity properties, we have to fix the meaning of the "normal" and "inverted"