linux-mmc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] mmc: Add sdhci workaround stability enhencement
@ 2019-12-02 14:41 Jun Nie
  2019-12-02 14:41 ` [PATCH 1/4] mmc: sdhci: Add delay after power off Jun Nie
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Jun Nie @ 2019-12-02 14:41 UTC (permalink / raw)
  To: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

This patch set made three changes:
 1. set enhence full power cycle stability.
 2. Add dt property to support DMA memory address boundary workaround.
 3. Add dt property to non-standard HS400 mode value in ctrl register.


Jun Nie (4):
  mmc: sdhci: Add delay after power off
  mmc: sdhci: dt: Add DMA boundary and HS400 properties
  mmc: sdhci: Set ctrl_hs400 value in dts
  mmc: sdhci: Add DMA memory boundary workaround

 .../devicetree/bindings/mmc/sdhci.txt         |  8 +++++
 drivers/mmc/host/sdhci.c                      | 36 +++++++++++++++++--
 drivers/mmc/host/sdhci.h                      |  9 ++++-
 3 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/4] mmc: sdhci: Add delay after power off
  2019-12-02 14:41 [PATCH 0/4] mmc: Add sdhci workaround stability enhencement Jun Nie
@ 2019-12-02 14:41 ` Jun Nie
  2019-12-03  7:26   ` Adrian Hunter
  2019-12-02 14:41 ` [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties Jun Nie
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 24+ messages in thread
From: Jun Nie @ 2019-12-02 14:41 UTC (permalink / raw)
  To: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

Add delay after power off to ensure that full power cycle is
successful. Otherwise, some controllers, at lease for Hisilicon
eMMC controller, may not be unstable sometimes for full power
cycle operation.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/mmc/host/sdhci.c | 7 +++++++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3140fe2e5dba..a654f0aeb438 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1761,6 +1761,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
 		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
 		if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON)
 			sdhci_runtime_pm_bus_off(host);
+
+		/*
+		 * Some controllers need an extra 100ms delay to secure
+		 * full power cycle is successful.
+		 */
+		if (host->quirks2 & SDHCI_QUIRK2_DELAY_AFTER_POWER_OFF)
+			msleep(100);
 	} else {
 		/*
 		 * Spec says that we should clear the power reg before setting
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0ed3e0eaef5f..0e6f97eaa796 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -482,6 +482,8 @@ struct sdhci_host {
  * block count.
  */
 #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
+/* Controllers need an extra 100ms delay to make sure power off completely */
+#define SDHCI_QUIRK2_DELAY_AFTER_POWER_OFF		(1<<19)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties
  2019-12-02 14:41 [PATCH 0/4] mmc: Add sdhci workaround stability enhencement Jun Nie
  2019-12-02 14:41 ` [PATCH 1/4] mmc: sdhci: Add delay after power off Jun Nie
@ 2019-12-02 14:41 ` Jun Nie
  2019-12-13 23:01   ` Rob Herring
  2019-12-02 14:41 ` [PATCH 3/4] mmc: sdhci: Set ctrl_hs400 value in dts Jun Nie
  2019-12-02 14:41 ` [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround Jun Nie
  3 siblings, 1 reply; 24+ messages in thread
From: Jun Nie @ 2019-12-02 14:41 UTC (permalink / raw)
  To: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

DMA memory cannot cross specific boundary on some controller, such as 128MB
on SDHCI Designware. Add sdhci-dma-mem-boundary property to split DMA
operation in such case.

sdhci-ctrl-hs400 specify the HS400 mode setting for register
SDHCI_HOST_CONTROL2(offset 0x3E:bit[2:0]). Because this value is not
defined in SDHC Standard specification.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 Documentation/devicetree/bindings/mmc/sdhci.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci.txt b/Documentation/devicetree/bindings/mmc/sdhci.txt
index 0e9923a64024..e6d7feb9a741 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci.txt
@@ -11,3 +11,11 @@ Optional properties:
 - sdhci-caps: The sdhci capabilities register is incorrect. This 64bit
   property corresponds to the bits in the sdhci capability register. If the
   bit is on in the property then the bit should be turned on.
+- sdhci-dma-mem-boundary: The sdhci controller DMA memory space boundary.
+  If the controller's DMA cannot cross a specific memory space boundary,
+  such as 128MB, set this value in dt and driver will split the DMA
+  operation when crossing such boundary.
+- sdhci-ctrl-hs400: The HS400 is not defined in SDHC Standard specification
+  for SDHCI_HOST_CONTROL2(offset 0x3E:bit[2:0]). Different controllers have
+  have different value for HS400 mode. If 0x5 is not the HS400 mode value
+  for your controller, you should specify the value with this property.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mmc: sdhci: Set ctrl_hs400 value in dts
  2019-12-02 14:41 [PATCH 0/4] mmc: Add sdhci workaround stability enhencement Jun Nie
  2019-12-02 14:41 ` [PATCH 1/4] mmc: sdhci: Add delay after power off Jun Nie
  2019-12-02 14:41 ` [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties Jun Nie
@ 2019-12-02 14:41 ` Jun Nie
  2019-12-03  7:52   ` Adrian Hunter
  2019-12-02 14:41 ` [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround Jun Nie
  3 siblings, 1 reply; 24+ messages in thread
From: Jun Nie @ 2019-12-02 14:41 UTC (permalink / raw)
  To: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

Because ctrl_hs400 value is non-standard, add support to set it
in dts.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/mmc/host/sdhci.c | 9 ++++++++-
 drivers/mmc/host/sdhci.h | 6 +++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a654f0aeb438..d8a6c1c91448 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1899,7 +1899,7 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 		 (timing == MMC_TIMING_MMC_DDR52))
 		ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
 	else if (timing == MMC_TIMING_MMC_HS400)
-		ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
+		ctrl_2 |= host->sdhci_ctrl_hs400; /* Non-standard */
 	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
 }
 EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
@@ -3635,6 +3635,13 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,
 	of_property_read_u64(mmc_dev(host->mmc)->of_node,
 			     "sdhci-caps", &dt_caps);
 
+	if (of_property_read_u32(mmc_dev(host->mmc)->of_node,
+				 "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400))
+		host->sdhci_ctrl_hs400 = SDHCI_CTRL_HS400;
+	else
+		WARN_ON(host->sdhci_ctrl_hs400 > 7
+			|| host->sdhci_ctrl_hs400 < 5);
+
 	v = ver ? *ver : sdhci_readw(host, SDHCI_HOST_VERSION);
 	host->version = (v & SDHCI_SPEC_VER_MASK) >> SDHCI_SPEC_VER_SHIFT;
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0e6f97eaa796..cac4d819f62c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -184,7 +184,8 @@
 #define   SDHCI_CTRL_UHS_SDR50		0x0002
 #define   SDHCI_CTRL_UHS_SDR104		0x0003
 #define   SDHCI_CTRL_UHS_DDR50		0x0004
-#define   SDHCI_CTRL_HS400		0x0005 /* Non-standard */
+/* SDHCI_CTRL_HS400 is non-standard value, can change it in dts */
+#define   SDHCI_CTRL_HS400		0x0005
 #define  SDHCI_CTRL_VDD_180		0x0008
 #define  SDHCI_CTRL_DRV_TYPE_MASK	0x0030
 #define   SDHCI_CTRL_DRV_TYPE_B		0x0000
@@ -605,6 +606,9 @@ struct sdhci_host {
 
 	u64			data_timeout;
 
+	/* SDHCI_CTRL_HS400 value */
+	u32			sdhci_ctrl_hs400;
+
 	unsigned long private[0] ____cacheline_aligned;
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-02 14:41 [PATCH 0/4] mmc: Add sdhci workaround stability enhencement Jun Nie
                   ` (2 preceding siblings ...)
  2019-12-02 14:41 ` [PATCH 3/4] mmc: sdhci: Set ctrl_hs400 value in dts Jun Nie
@ 2019-12-02 14:41 ` Jun Nie
  2019-12-02 17:52   ` Christoph Hellwig
  2019-12-03  2:47   ` Jisheng Zhang
  3 siblings, 2 replies; 24+ messages in thread
From: Jun Nie @ 2019-12-02 14:41 UTC (permalink / raw)
  To: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

DMA memory cannot cross specific boundary for some SDHCI controller,
such as DesignWare SDHCI controller. Add DMA memory boundary dt
property and workaround the limitation.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/mmc/host/sdhci.c | 20 +++++++++++++++++++-
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d8a6c1c91448..56c53fbadd9d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -763,9 +763,25 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 		BUG_ON(len > 65536);
 
 		/* tran, valid */
-		if (len)
+		if (len) {
+			unsigned int boundary = host->dma_mem_boundary;
+			/*
+			 * work around for buffer across mem boundary, split
+			 * the buffer.
+			 */
+			if (boundary &&
+			    ((addr & (boundary - 1)) + len) > boundary) {
+				offset = boundary - (addr & (boundary - 1));
+				__sdhci_adma_write_desc(host, &desc,
+							addr, offset,
+							ADMA2_TRAN_VALID);
+				addr += offset;
+				len -= offset;
+			}
+
 			__sdhci_adma_write_desc(host, &desc, addr, len,
 						ADMA2_TRAN_VALID);
+		}
 
 		/*
 		 * If this triggers then we have a calculation bug
@@ -3634,6 +3650,8 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,
 			     "sdhci-caps-mask", &dt_caps_mask);
 	of_property_read_u64(mmc_dev(host->mmc)->of_node,
 			     "sdhci-caps", &dt_caps);
+	of_property_read_u32(mmc_dev(host->mmc)->of_node,
+			     "sdhci-dma-mem-boundary", &host->dma_mem_boundary);
 
 	if (of_property_read_u32(mmc_dev(host->mmc)->of_node,
 				 "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400))
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cac4d819f62c..954ac08c4fb0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -608,6 +608,7 @@ struct sdhci_host {
 
 	/* SDHCI_CTRL_HS400 value */
 	u32			sdhci_ctrl_hs400;
+	u32			dma_mem_boundary;
 
 	unsigned long private[0] ____cacheline_aligned;
 };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-02 14:41 ` [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround Jun Nie
@ 2019-12-02 17:52   ` Christoph Hellwig
  2019-12-03  3:29     ` Jun Nie
  2019-12-03  2:47   ` Jisheng Zhang
  1 sibling, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2019-12-02 17:52 UTC (permalink / raw)
  To: Jun Nie
  Cc: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

On Mon, Dec 02, 2019 at 10:41:04PM +0800, Jun Nie wrote:
> DMA memory cannot cross specific boundary for some SDHCI controller,
> such as DesignWare SDHCI controller. Add DMA memory boundary dt
> property and workaround the limitation.

If you use blk_queue_segment_boundary to tell the block layer the
segment boundary restrictions it won't ever send you segments
that don't fit.  With just the workaround in this patch you'll run into
max_segments accounting issues, don't you?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-02 14:41 ` [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround Jun Nie
  2019-12-02 17:52   ` Christoph Hellwig
@ 2019-12-03  2:47   ` Jisheng Zhang
  2019-12-03  3:33     ` Jun Nie
  1 sibling, 1 reply; 24+ messages in thread
From: Jisheng Zhang @ 2019-12-03  2:47 UTC (permalink / raw)
  To: Jun Nie
  Cc: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:


> 
> 
> DMA memory cannot cross specific boundary for some SDHCI controller,
> such as DesignWare SDHCI controller. Add DMA memory boundary dt
> property and workaround the limitation.

IMHO, the workaround could be implemented in each SDHCI host driver.

eg. drivers/mmc/host/sdhci-of-dwcmshc.c

thanks

> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c | 20 +++++++++++++++++++-
>  drivers/mmc/host/sdhci.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index d8a6c1c91448..56c53fbadd9d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -763,9 +763,25 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
>                 BUG_ON(len > 65536);
> 
>                 /* tran, valid */
> -               if (len)
> +               if (len) {
> +                       unsigned int boundary = host->dma_mem_boundary;
> +                       /*
> +                        * work around for buffer across mem boundary, split
> +                        * the buffer.
> +                        */
> +                       if (boundary &&
> +                           ((addr & (boundary - 1)) + len) > boundary) {
> +                               offset = boundary - (addr & (boundary - 1));
> +                               __sdhci_adma_write_desc(host, &desc,
> +                                                       addr, offset,
> +                                                       ADMA2_TRAN_VALID);
> +                               addr += offset;
> +                               len -= offset;
> +                       }
> +
>                         __sdhci_adma_write_desc(host, &desc, addr, len,
>                                                 ADMA2_TRAN_VALID);
> +               }
> 
>                 /*
>                  * If this triggers then we have a calculation bug
> @@ -3634,6 +3650,8 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,
>                              "sdhci-caps-mask", &dt_caps_mask);
>         of_property_read_u64(mmc_dev(host->mmc)->of_node,
>                              "sdhci-caps", &dt_caps);
> +       of_property_read_u32(mmc_dev(host->mmc)->of_node,
> +                            "sdhci-dma-mem-boundary", &host->dma_mem_boundary);
> 
>         if (of_property_read_u32(mmc_dev(host->mmc)->of_node,
>                                  "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400))
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index cac4d819f62c..954ac08c4fb0 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -608,6 +608,7 @@ struct sdhci_host {
> 
>         /* SDHCI_CTRL_HS400 value */
>         u32                     sdhci_ctrl_hs400;
> +       u32                     dma_mem_boundary;
> 
>         unsigned long private[0] ____cacheline_aligned;
>  };
> --
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-02 17:52   ` Christoph Hellwig
@ 2019-12-03  3:29     ` Jun Nie
  2019-12-03  7:36       ` Christoph Hellwig
  0 siblings, 1 reply; 24+ messages in thread
From: Jun Nie @ 2019-12-03  3:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ulf Hansson, Rob Herring, Mark Rutland, adrian.hunter, linux-mmc,
	devicetree

Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 上午1:52写道:
>
> On Mon, Dec 02, 2019 at 10:41:04PM +0800, Jun Nie wrote:
> > DMA memory cannot cross specific boundary for some SDHCI controller,
> > such as DesignWare SDHCI controller. Add DMA memory boundary dt
> > property and workaround the limitation.
>
> If you use blk_queue_segment_boundary to tell the block layer the
> segment boundary restrictions it won't ever send you segments
> that don't fit.  With just the workaround in this patch you'll run into
> max_segments accounting issues, don't you?

Thanks for the reminder! So I need to parse the segment_boundary from
device tree and use below code to set it, right?
For the max_segments accounting error, I did not see it so far though I
believe it is true in theory. Maybe it is due to segment boundary value is
very large.

+++ b/drivers/mmc/core/queue.c
@@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,
struct mmc_card *card)
                WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
                                                        mmc_dev(host)),
                     "merging was advertised but not possible");
+       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);
        blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  2:47   ` Jisheng Zhang
@ 2019-12-03  3:33     ` Jun Nie
  2019-12-03  9:05       ` Jisheng Zhang
  0 siblings, 1 reply; 24+ messages in thread
From: Jun Nie @ 2019-12-03  3:33 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

Jisheng Zhang <Jisheng.Zhang@synaptics.com> 于2019年12月3日周二 上午10:47写道:
>
> On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:
>
>
> >
> >
> > DMA memory cannot cross specific boundary for some SDHCI controller,
> > such as DesignWare SDHCI controller. Add DMA memory boundary dt
> > property and workaround the limitation.
>
> IMHO, the workaround could be implemented in each SDHCI host driver.
>
> eg. drivers/mmc/host/sdhci-of-dwcmshc.c
>
Thanks for the suggestion! Christoph's suggestion can prevent the the issue
from the block layer, thus the code can be shared across all
controllers. I prefer
his suggestions.

Jun

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] mmc: sdhci: Add delay after power off
  2019-12-02 14:41 ` [PATCH 1/4] mmc: sdhci: Add delay after power off Jun Nie
@ 2019-12-03  7:26   ` Adrian Hunter
  0 siblings, 0 replies; 24+ messages in thread
From: Adrian Hunter @ 2019-12-03  7:26 UTC (permalink / raw)
  To: Jun Nie, ulf.hansson, robh+dt, mark.rutland, linux-mmc, devicetree

On 2/12/19 4:41 pm, Jun Nie wrote:
> Add delay after power off to ensure that full power cycle is
> successful. Otherwise, some controllers, at lease for Hisilicon
> eMMC controller, may not be unstable sometimes for full power
> cycle operation.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c | 7 +++++++
>  drivers/mmc/host/sdhci.h | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 3140fe2e5dba..a654f0aeb438 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1761,6 +1761,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
>  		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>  		if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON)
>  			sdhci_runtime_pm_bus_off(host);
> +
> +		/*
> +		 * Some controllers need an extra 100ms delay to secure
> +		 * full power cycle is successful.
> +		 */
> +		if (host->quirks2 & SDHCI_QUIRK2_DELAY_AFTER_POWER_OFF)
> +			msleep(100);

Please use the ->set_power() callback and do this in your own driver.

>  	} else {
>  		/*
>  		 * Spec says that we should clear the power reg before setting
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0ed3e0eaef5f..0e6f97eaa796 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -482,6 +482,8 @@ struct sdhci_host {
>   * block count.
>   */
>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
> +/* Controllers need an extra 100ms delay to make sure power off completely */
> +#define SDHCI_QUIRK2_DELAY_AFTER_POWER_OFF		(1<<19)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  3:29     ` Jun Nie
@ 2019-12-03  7:36       ` Christoph Hellwig
  2019-12-04  6:00         ` Jun Nie
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2019-12-03  7:36 UTC (permalink / raw)
  To: Jun Nie
  Cc: Christoph Hellwig, Ulf Hansson, Rob Herring, Mark Rutland,
	adrian.hunter, linux-mmc, devicetree

On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:
> Thanks for the reminder! So I need to parse the segment_boundary from
> device tree and use below code to set it, right?
> For the max_segments accounting error, I did not see it so far though I
> believe it is true in theory. Maybe it is due to segment boundary value is
> very large.
>
> +++ b/drivers/mmc/core/queue.c
> @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,
> struct mmc_card *card)
>                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
>                                                         mmc_dev(host)),
>                      "merging was advertised but not possible");
> +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);
>         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));

Yes, I think should do it.  Maybe modulo a check if the low-level
driver actually sets a segment boundary.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/4] mmc: sdhci: Set ctrl_hs400 value in dts
  2019-12-02 14:41 ` [PATCH 3/4] mmc: sdhci: Set ctrl_hs400 value in dts Jun Nie
@ 2019-12-03  7:52   ` Adrian Hunter
  0 siblings, 0 replies; 24+ messages in thread
From: Adrian Hunter @ 2019-12-03  7:52 UTC (permalink / raw)
  To: Jun Nie, ulf.hansson, robh+dt, mark.rutland, linux-mmc, devicetree

On 2/12/19 4:41 pm, Jun Nie wrote:
> Because ctrl_hs400 value is non-standard, add support to set it
> in dts.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c | 9 ++++++++-
>  drivers/mmc/host/sdhci.h | 6 +++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a654f0aeb438..d8a6c1c91448 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1899,7 +1899,7 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>  		 (timing == MMC_TIMING_MMC_DDR52))
>  		ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>  	else if (timing == MMC_TIMING_MMC_HS400)
> -		ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
> +		ctrl_2 |= host->sdhci_ctrl_hs400; /* Non-standard */

Let's limit this to the correct bits i.e.

		ctrl_2 |= host->sdhci_ctrl_hs400 & 7; /* Non-standard */

>  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
> @@ -3635,6 +3635,13 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,
>  	of_property_read_u64(mmc_dev(host->mmc)->of_node,
>  			     "sdhci-caps", &dt_caps);

Would you mind changing these APIs from 'of' to 'dev' properties?
Needs to be a separate patch.

>  
> +	if (of_property_read_u32(mmc_dev(host->mmc)->of_node,
> +				 "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400))

And then use a 'dev' property API here.

> +		host->sdhci_ctrl_hs400 = SDHCI_CTRL_HS400;
> +	else
> +		WARN_ON(host->sdhci_ctrl_hs400 > 7
> +			|| host->sdhci_ctrl_hs400 < 5);

Please remove this warning.

> +
>  	v = ver ? *ver : sdhci_readw(host, SDHCI_HOST_VERSION);
>  	host->version = (v & SDHCI_SPEC_VER_MASK) >> SDHCI_SPEC_VER_SHIFT;
>  
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0e6f97eaa796..cac4d819f62c 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -184,7 +184,8 @@
>  #define   SDHCI_CTRL_UHS_SDR50		0x0002
>  #define   SDHCI_CTRL_UHS_SDR104		0x0003
>  #define   SDHCI_CTRL_UHS_DDR50		0x0004
> -#define   SDHCI_CTRL_HS400		0x0005 /* Non-standard */
> +/* SDHCI_CTRL_HS400 is non-standard value, can change it in dts */
> +#define   SDHCI_CTRL_HS400		0x0005
>  #define  SDHCI_CTRL_VDD_180		0x0008
>  #define  SDHCI_CTRL_DRV_TYPE_MASK	0x0030
>  #define   SDHCI_CTRL_DRV_TYPE_B		0x0000
> @@ -605,6 +606,9 @@ struct sdhci_host {
>  
>  	u64			data_timeout;
>  
> +	/* SDHCI_CTRL_HS400 value */
> +	u32			sdhci_ctrl_hs400;
> +
>  	unsigned long private[0] ____cacheline_aligned;
>  };
>  
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  3:33     ` Jun Nie
@ 2019-12-03  9:05       ` Jisheng Zhang
  2019-12-03  9:18         ` Christoph Hellwig
  2019-12-03  9:49         ` Jisheng Zhang
  0 siblings, 2 replies; 24+ messages in thread
From: Jisheng Zhang @ 2019-12-03  9:05 UTC (permalink / raw)
  To: Jun Nie, Christoph Hellwig
  Cc: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

+ Christoph

On Tue, 3 Dec 2019 11:33:15 +0800 Jun Nie wrote:

> 
> 
> Jisheng Zhang <Jisheng.Zhang@synaptics.com> 于2019年12月3日周二 上午10:47写道:
> >
> > On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:
> >
> >  
> > >
> > >
> > > DMA memory cannot cross specific boundary for some SDHCI controller,
> > > such as DesignWare SDHCI controller. Add DMA memory boundary dt
> > > property and workaround the limitation.  
> >
> > IMHO, the workaround could be implemented in each SDHCI host driver.
> >
> > eg. drivers/mmc/host/sdhci-of-dwcmshc.c
> >  
> Thanks for the suggestion! Christoph's suggestion can prevent the the issue
> from the block layer, thus the code can be shared across all

To be honest, I did consider similar solution from block layer, I.E set
the seg_boundary_mask, when submitting the workaround last year, but per
my understanding, SDHCI limitation is the physical DMA addr can't span one
specific boundary, so setting seg_boundary_mask w/ blk_queue_segment_boundary
can't work. I'm not sure I understand blk_queue_segment_boundary() properly.
May Christoph help to clarify?

From another side, drivers/ata/libata-sff.c also workaround the 64K phy DMA
boundary limitation itself rather than from block layer.

Thanks

> controllers. I prefer
> his suggestions.
> 
> Jun


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  9:05       ` Jisheng Zhang
@ 2019-12-03  9:18         ` Christoph Hellwig
  2019-12-03  9:49           ` Jisheng Zhang
  2019-12-03  9:49         ` Jisheng Zhang
  1 sibling, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2019-12-03  9:18 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Jun Nie, Christoph Hellwig, ulf.hansson, robh+dt, mark.rutland,
	adrian.hunter, linux-mmc, devicetree, linux-block, linux-ide

On Tue, Dec 03, 2019 at 09:05:23AM +0000, Jisheng Zhang wrote:
> > >
> > > eg. drivers/mmc/host/sdhci-of-dwcmshc.c
> > >  
> > Thanks for the suggestion! Christoph's suggestion can prevent the the issue
> > from the block layer, thus the code can be shared across all
> 
> To be honest, I did consider similar solution from block layer, I.E set
> the seg_boundary_mask, when submitting the workaround last year, but per
> my understanding, SDHCI limitation is the physical DMA addr can't span one
> specific boundary,

As in exactly one boundary and not an alignment?  Where the one
boundary is not a power of two and thus can't be expressed?


> so setting seg_boundary_mask w/ blk_queue_segment_boundary
> can't work. I'm not sure I understand blk_queue_segment_boundary() properly.
> May Christoph help to clarify?
> 
> From another side, drivers/ata/libata-sff.c also workaround the 64K phy DMA
> boundary limitation itself rather than from block layer.

As far as I can tell that workaround should use the segment boundary
setting as well.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  9:05       ` Jisheng Zhang
  2019-12-03  9:18         ` Christoph Hellwig
@ 2019-12-03  9:49         ` Jisheng Zhang
  2019-12-03 13:04           ` Christoph Hellwig
  1 sibling, 1 reply; 24+ messages in thread
From: Jisheng Zhang @ 2019-12-03  9:49 UTC (permalink / raw)
  To: Jun Nie, Christoph Hellwig
  Cc: ulf.hansson, robh+dt, mark.rutland, adrian.hunter, linux-mmc, devicetree

On Tue, 3 Dec 2019 09:05:23 +0000 Jisheng Zhang wrote:


> 
> + Christoph
> 
> On Tue, 3 Dec 2019 11:33:15 +0800 Jun Nie wrote:
> 
> >
> >
> > Jisheng Zhang <Jisheng.Zhang@synaptics.com> 于2019年12月3日周二 上午10:47写道:  
> > >
> > > On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:
> > >
> > >  
> > > >
> > > >
> > > > DMA memory cannot cross specific boundary for some SDHCI controller,
> > > > such as DesignWare SDHCI controller. Add DMA memory boundary dt
> > > > property and workaround the limitation.  
> > >
> > > IMHO, the workaround could be implemented in each SDHCI host driver.
> > >
> > > eg. drivers/mmc/host/sdhci-of-dwcmshc.c
> > >  
> > Thanks for the suggestion! Christoph's suggestion can prevent the the issue
> > from the block layer, thus the code can be shared across all  
> 
> To be honest, I did consider similar solution from block layer, I.E set
> the seg_boundary_mask, when submitting the workaround last year, but per
> my understanding, SDHCI limitation is the physical DMA addr can't span one
> specific boundary, so setting seg_boundary_mask w/ blk_queue_segment_boundary
> can't work. I'm not sure I understand blk_queue_segment_boundary() properly.
> May Christoph help to clarify?

what's more, not all scatterlist in mmc are from block layer, for example, 
__mmc_blk_ioctl_cmd(), mmc_test.c etc..

How do we ensure the boundary is fine in these cases?

> 
> From another side, drivers/ata/libata-sff.c also workaround the 64K phy DMA
> boundary limitation itself rather than from block layer.

> Thanks
> 
> > controllers. I prefer
> > his suggestions.
> >
> > Jun  
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  9:18         ` Christoph Hellwig
@ 2019-12-03  9:49           ` Jisheng Zhang
  2019-12-03 13:06             ` Christoph Hellwig
  0 siblings, 1 reply; 24+ messages in thread
From: Jisheng Zhang @ 2019-12-03  9:49 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jun Nie, ulf.hansson, robh+dt, mark.rutland, adrian.hunter,
	linux-mmc, devicetree, linux-block, linux-ide

On Tue, 3 Dec 2019 01:18:24 -0800 Christoph Hellwig <hch@infradead.org> wrote:


> 
> 
> On Tue, Dec 03, 2019 at 09:05:23AM +0000, Jisheng Zhang wrote:
> > > >
> > > > eg. drivers/mmc/host/sdhci-of-dwcmshc.c
> > > >  
> > > Thanks for the suggestion! Christoph's suggestion can prevent the the issue
> > > from the block layer, thus the code can be shared across all  
> >
> > To be honest, I did consider similar solution from block layer, I.E set
> > the seg_boundary_mask, when submitting the workaround last year, but per
> > my understanding, SDHCI limitation is the physical DMA addr can't span one
> > specific boundary,  
> 
> As in exactly one boundary and not an alignment?  Where the one
> boundary is not a power of two and thus can't be expressed?

Take drivers/mmc/host/sdhci-of-dwcmshc.c for example, target physical DMA addr
can't span 128MB, 256MB, 128*3MB, ...128*nMB

I'm not sure whether blk_queue_segment_boundary could solve this limitation.

> 
> 
> > so setting seg_boundary_mask w/ blk_queue_segment_boundary
> > can't work. I'm not sure I understand blk_queue_segment_boundary() properly.
> > May Christoph help to clarify?
> >
> > From another side, drivers/ata/libata-sff.c also workaround the 64K phy DMA
> > boundary limitation itself rather than from block layer.  
> 
> As far as I can tell that workaround should use the segment boundary
> setting as well.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  9:49         ` Jisheng Zhang
@ 2019-12-03 13:04           ` Christoph Hellwig
  0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2019-12-03 13:04 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Jun Nie, Christoph Hellwig, ulf.hansson, robh+dt, mark.rutland,
	adrian.hunter, linux-mmc, devicetree

On Tue, Dec 03, 2019 at 09:49:33AM +0000, Jisheng Zhang wrote:
> > can't work. I'm not sure I understand blk_queue_segment_boundary() properly.
> > May Christoph help to clarify?
> 
> what's more, not all scatterlist in mmc are from block layer, for example, 
> __mmc_blk_ioctl_cmd(), mmc_test.c etc..
> 
> How do we ensure the boundary is fine in these cases?

By using block layer passthrough requests (REQ_OP_DRV*).  Otherwise
any other I/O limit imposed by the driver is ignored as well.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  9:49           ` Jisheng Zhang
@ 2019-12-03 13:06             ` Christoph Hellwig
  2019-12-04  7:11               ` Jisheng Zhang
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2019-12-03 13:06 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Christoph Hellwig, Jun Nie, ulf.hansson, robh+dt, mark.rutland,
	adrian.hunter, linux-mmc, devicetree, linux-block, linux-ide

On Tue, Dec 03, 2019 at 09:49:49AM +0000, Jisheng Zhang wrote:
> > As in exactly one boundary and not an alignment?  Where the one
> > boundary is not a power of two and thus can't be expressed?
> 
> Take drivers/mmc/host/sdhci-of-dwcmshc.c for example, target physical DMA addr
> can't span 128MB, 256MB, 128*3MB, ...128*nMB
> 
> I'm not sure whether blk_queue_segment_boundary could solve this limitation.

That is exaxtly the kind of limitation blk_queue_segment_boundary is
intended for.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03  7:36       ` Christoph Hellwig
@ 2019-12-04  6:00         ` Jun Nie
  2019-12-04  7:14           ` Jisheng Zhang
  0 siblings, 1 reply; 24+ messages in thread
From: Jun Nie @ 2019-12-04  6:00 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ulf Hansson, Rob Herring, Mark Rutland, adrian.hunter, linux-mmc,
	devicetree

Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 下午3:36写道:
>
> On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:
> > Thanks for the reminder! So I need to parse the segment_boundary from
> > device tree and use below code to set it, right?
> > For the max_segments accounting error, I did not see it so far though I
> > believe it is true in theory. Maybe it is due to segment boundary value is
> > very large.
> >
> > +++ b/drivers/mmc/core/queue.c
> > @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,
> > struct mmc_card *card)
> >                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
> >                                                         mmc_dev(host)),
> >                      "merging was advertised but not possible");
> > +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);
> >         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>
> Yes, I think should do it.  Maybe modulo a check if the low-level
> driver actually sets a segment boundary.

For the block device, such as SD card, it is right solution. But I
have concern on SDIO case. Maybe we should add workaround together
with block layer segment boundary restriction. How do you think about
it?

Jun

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-03 13:06             ` Christoph Hellwig
@ 2019-12-04  7:11               ` Jisheng Zhang
  0 siblings, 0 replies; 24+ messages in thread
From: Jisheng Zhang @ 2019-12-04  7:11 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jun Nie, ulf.hansson, robh+dt, mark.rutland, adrian.hunter,
	linux-mmc, devicetree, linux-block, linux-ide

Hi Christoph

On Tue, 3 Dec 2019 05:06:09 -0800 Christoph Hellwig wrote:


> 
> On Tue, Dec 03, 2019 at 09:49:49AM +0000, Jisheng Zhang wrote:
> > > As in exactly one boundary and not an alignment?  Where the one
> > > boundary is not a power of two and thus can't be expressed?  
> >
> > Take drivers/mmc/host/sdhci-of-dwcmshc.c for example, target physical DMA addr
> > can't span 128MB, 256MB, 128*3MB, ...128*nMB
> >
> > I'm not sure whether blk_queue_segment_boundary could solve this limitation.  
> 
> That is exaxtly the kind of limitation blk_queue_segment_boundary is
> intended for.

Until after dma_map_sg(), we can't know the physical DMA address range, so
how does block layer know and check the DMA range beforehand? I'm a newbie on
block layer, could you please teach me? At the same time
I'm reading the code as well.

Thanks in advance


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-04  6:00         ` Jun Nie
@ 2019-12-04  7:14           ` Jisheng Zhang
  2019-12-10  9:36             ` Ulf Hansson
  0 siblings, 1 reply; 24+ messages in thread
From: Jisheng Zhang @ 2019-12-04  7:14 UTC (permalink / raw)
  To: Jun Nie
  Cc: Christoph Hellwig, Ulf Hansson, Rob Herring, Mark Rutland,
	adrian.hunter, linux-mmc, devicetree

On Wed, 4 Dec 2019 14:00:08 +0800 Jun Nie wrote:

> 
> 
> Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 下午3:36写道:
> >
> > On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:  
> > > Thanks for the reminder! So I need to parse the segment_boundary from
> > > device tree and use below code to set it, right?
> > > For the max_segments accounting error, I did not see it so far though I
> > > believe it is true in theory. Maybe it is due to segment boundary value is
> > > very large.
> > >
> > > +++ b/drivers/mmc/core/queue.c
> > > @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,
> > > struct mmc_card *card)
> > >                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
> > >                                                         mmc_dev(host)),
> > >                      "merging was advertised but not possible");
> > > +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);
> > >         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));  
> >
> > Yes, I think should do it.  Maybe modulo a check if the low-level
> > driver actually sets a segment boundary.  
> 
> For the block device, such as SD card, it is right solution. But I
> have concern on SDIO case. Maybe we should add workaround together
> with block layer segment boundary restriction. How do you think about
> it?
> 

Another trouble is how to workaround if the sg is constructed by mmc and
no block layer interactions at all. e.g __mmc_blk_ioctl_cmd(), and all
those sgs in mmc_test.c

Thanks

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround
  2019-12-04  7:14           ` Jisheng Zhang
@ 2019-12-10  9:36             ` Ulf Hansson
  0 siblings, 0 replies; 24+ messages in thread
From: Ulf Hansson @ 2019-12-10  9:36 UTC (permalink / raw)
  To: Jisheng Zhang, Christoph Hellwig
  Cc: Jun Nie, Rob Herring, Mark Rutland, adrian.hunter, linux-mmc, devicetree

On Wed, 4 Dec 2019 at 08:14, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>
> On Wed, 4 Dec 2019 14:00:08 +0800 Jun Nie wrote:
>
> >
> >
> > Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 下午3:36写道:
> > >
> > > On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:
> > > > Thanks for the reminder! So I need to parse the segment_boundary from
> > > > device tree and use below code to set it, right?
> > > > For the max_segments accounting error, I did not see it so far though I
> > > > believe it is true in theory. Maybe it is due to segment boundary value is
> > > > very large.
> > > >
> > > > +++ b/drivers/mmc/core/queue.c
> > > > @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,
> > > > struct mmc_card *card)
> > > >                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
> > > >                                                         mmc_dev(host)),
> > > >                      "merging was advertised but not possible");
> > > > +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);
> > > >         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> > >
> > > Yes, I think should do it.  Maybe modulo a check if the low-level
> > > driver actually sets a segment boundary.
> >
> > For the block device, such as SD card, it is right solution. But I
> > have concern on SDIO case. Maybe we should add workaround together
> > with block layer segment boundary restriction. How do you think about
> > it?
> >

Yes, buffers for SDIO are a consern. Especially since those buffers
are allocated by SDIO func drivers or even from upper layers, such as
the network stacks, for example.

I think SDIO func drivers simply need to respect the constraints the
host has set via the .segment_boundary, .max_seg_size, .max_segs, etc.
We should export SDIO func APIs to make the information available for
the SDIO func drivers.

>
> Another trouble is how to workaround if the sg is constructed by mmc and
> no block layer interactions at all. e.g __mmc_blk_ioctl_cmd(), and all
> those sgs in mmc_test.c

Those should be easier to fix, as the buffer/sg allocation can be
fixed internally by mmc core. Just post some patches. :-)

I am more worried about SDIO, as those buffers are not that easy to control.

Note that, there have been suggestions on adding an SDIO interface
where an sg can be passed [1]. Unfurtunate those patches got stuck and
didn't make it.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/10123143/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties
  2019-12-02 14:41 ` [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties Jun Nie
@ 2019-12-13 23:01   ` Rob Herring
  2019-12-17 14:56     ` Jun Nie
  0 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2019-12-13 23:01 UTC (permalink / raw)
  To: Jun Nie; +Cc: ulf.hansson, mark.rutland, adrian.hunter, linux-mmc, devicetree

On Mon, Dec 02, 2019 at 10:41:02PM +0800, Jun Nie wrote:
> DMA memory cannot cross specific boundary on some controller, such as 128MB
> on SDHCI Designware. Add sdhci-dma-mem-boundary property to split DMA
> operation in such case.
> 
> sdhci-ctrl-hs400 specify the HS400 mode setting for register
> SDHCI_HOST_CONTROL2(offset 0x3E:bit[2:0]). Because this value is not
> defined in SDHC Standard specification.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci.txt b/Documentation/devicetree/bindings/mmc/sdhci.txt
> index 0e9923a64024..e6d7feb9a741 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci.txt
> @@ -11,3 +11,11 @@ Optional properties:
>  - sdhci-caps: The sdhci capabilities register is incorrect. This 64bit
>    property corresponds to the bits in the sdhci capability register. If the
>    bit is on in the property then the bit should be turned on.
> +- sdhci-dma-mem-boundary: The sdhci controller DMA memory space boundary.
> +  If the controller's DMA cannot cross a specific memory space boundary,
> +  such as 128MB, set this value in dt and driver will split the DMA
> +  operation when crossing such boundary.

This should be implied by the compatible string.

> +- sdhci-ctrl-hs400: The HS400 is not defined in SDHC Standard specification
> +  for SDHCI_HOST_CONTROL2(offset 0x3E:bit[2:0]). Different controllers have
> +  have different value for HS400 mode. If 0x5 is not the HS400 mode value
> +  for your controller, you should specify the value with this property.

This too, unless it needs to be tuned per board.

Can you be more specific as to what the possible values are and what 
they do?

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties
  2019-12-13 23:01   ` Rob Herring
@ 2019-12-17 14:56     ` Jun Nie
  0 siblings, 0 replies; 24+ messages in thread
From: Jun Nie @ 2019-12-17 14:56 UTC (permalink / raw)
  To: Rob Herring
  Cc: Ulf Hansson, Mark Rutland, adrian.hunter, linux-mmc, devicetree

Rob Herring <robh@kernel.org> 于2019年12月14日周六 上午7:01写道:
>
> On Mon, Dec 02, 2019 at 10:41:02PM +0800, Jun Nie wrote:
> > DMA memory cannot cross specific boundary on some controller, such as 128MB
> > on SDHCI Designware. Add sdhci-dma-mem-boundary property to split DMA
> > operation in such case.
> >
> > sdhci-ctrl-hs400 specify the HS400 mode setting for register
> > SDHCI_HOST_CONTROL2(offset 0x3E:bit[2:0]). Because this value is not
> > defined in SDHC Standard specification.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/mmc/sdhci.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci.txt b/Documentation/devicetree/bindings/mmc/sdhci.txt
> > index 0e9923a64024..e6d7feb9a741 100644
> > --- a/Documentation/devicetree/bindings/mmc/sdhci.txt
> > +++ b/Documentation/devicetree/bindings/mmc/sdhci.txt
> > @@ -11,3 +11,11 @@ Optional properties:
> >  - sdhci-caps: The sdhci capabilities register is incorrect. This 64bit
> >    property corresponds to the bits in the sdhci capability register. If the
> >    bit is on in the property then the bit should be turned on.
> > +- sdhci-dma-mem-boundary: The sdhci controller DMA memory space boundary.
> > +  If the controller's DMA cannot cross a specific memory space boundary,
> > +  such as 128MB, set this value in dt and driver will split the DMA
> > +  operation when crossing such boundary.
>
> This should be implied by the compatible string.
>
> > +- sdhci-ctrl-hs400: The HS400 is not defined in SDHC Standard specification
> > +  for SDHCI_HOST_CONTROL2(offset 0x3E:bit[2:0]). Different controllers have
> > +  have different value for HS400 mode. If 0x5 is not the HS400 mode value
> > +  for your controller, you should specify the value with this property.
>
> This too, unless it needs to be tuned per board.
>
> Can you be more specific as to what the possible values are and what
> they do?

It is specific to SoC or specific to controller. HS400 mode value on
DWC3 of new Hisilicon
 SoC is 7, not 5. This same is to DMA buffer memory address boundary.
Do you mean you
want the boundary value and HS400 mode value should bundled with compatible,
ie. specific SoC or controller, to set a value in sdhci layer from
platform glue driver?

>
> Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-12-17 14:56 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 14:41 [PATCH 0/4] mmc: Add sdhci workaround stability enhencement Jun Nie
2019-12-02 14:41 ` [PATCH 1/4] mmc: sdhci: Add delay after power off Jun Nie
2019-12-03  7:26   ` Adrian Hunter
2019-12-02 14:41 ` [PATCH 2/4] mmc: sdhci: dt: Add DMA boundary and HS400 properties Jun Nie
2019-12-13 23:01   ` Rob Herring
2019-12-17 14:56     ` Jun Nie
2019-12-02 14:41 ` [PATCH 3/4] mmc: sdhci: Set ctrl_hs400 value in dts Jun Nie
2019-12-03  7:52   ` Adrian Hunter
2019-12-02 14:41 ` [PATCH 4/4] mmc: sdhci: Add DMA memory boundary workaround Jun Nie
2019-12-02 17:52   ` Christoph Hellwig
2019-12-03  3:29     ` Jun Nie
2019-12-03  7:36       ` Christoph Hellwig
2019-12-04  6:00         ` Jun Nie
2019-12-04  7:14           ` Jisheng Zhang
2019-12-10  9:36             ` Ulf Hansson
2019-12-03  2:47   ` Jisheng Zhang
2019-12-03  3:33     ` Jun Nie
2019-12-03  9:05       ` Jisheng Zhang
2019-12-03  9:18         ` Christoph Hellwig
2019-12-03  9:49           ` Jisheng Zhang
2019-12-03 13:06             ` Christoph Hellwig
2019-12-04  7:11               ` Jisheng Zhang
2019-12-03  9:49         ` Jisheng Zhang
2019-12-03 13:04           ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).