[PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Neil Armstrong]

The memory at the end of the controller only accepts 32bit read/write
accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
(which will be split into two 32bit access) and 8bit leading to incomplete
copies to/from this memory when the buffer is not multiple of 8bytes.

Add a local copy using writel/readl accesses to make sure we use the right
memory access width.

The switch to memcpy_to/fromio was done because of 285133040e6c
("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
worked before since it mainly used 32bit memory acceses.

Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
Reported-by: Christian Hewitt <christianshewitt@gmail.com>
Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 3f28eb4d17fe..08c0ff0bfa8b 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
 	writel(start, host->regs + SD_EMMC_START);
 }
 
-/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
+/* local sg copy for dram_access_quirk */
 static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
 				  size_t buflen, bool to_buffer)
 {
@@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
 	sg_miter_start(&miter, sgl, nents, sg_flags);
 
 	while ((offset < buflen) && sg_miter_next(&miter)) {
-		unsigned int len;
+		unsigned int buf_offset = 0;
+		unsigned int len, left;
+		u32 *buf = miter.addr;
+
+		if (((unsigned long int)miter.addr % 4))
+			dev_err(host->dev, "non word aligned sg");
 
 		len = min(miter.length, buflen - offset);
 
-		/* When dram_access_quirk, the bounce buffer is a iomem mapping */
-		if (host->dram_access_quirk) {
-			if (to_buffer)
-				memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
-			else
-				memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
+		if ((len % 4))
+			dev_err(host->dev, "non word multiple sg");
+
+		left = len;
+
+		if (to_buffer) {
+			do {
+				writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
+
+				buf_offset += 4;
+				left -= 4;
+			} while (left);
 		} else {
-			if (to_buffer)
-				memcpy(host->bounce_buf + offset, miter.addr, len);
-			else
-				memcpy(miter.addr, host->bounce_buf + offset, len);
+			do {
+				*buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
+
+				buf_offset += 4;
+				left -= 4;
+			} while (left);
 		}
 
 		offset += len;
@@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 		if (data->flags & MMC_DATA_WRITE) {
 			cmd_cfg |= CMD_CFG_DATA_WR;
 			WARN_ON(xfer_bytes > host->bounce_buf_size);
-			meson_mmc_copy_buffer(host, data, xfer_bytes, true);
+			if (host->dram_access_quirk)
+				meson_mmc_copy_buffer(host, data, xfer_bytes, true);
+			else
+				sg_copy_to_buffer(data->sg, data->sg_len,
+						  host->bounce_buf, xfer_bytes);
 			dma_wmb();
 		}
 
@@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 	if (meson_mmc_bounce_buf_read(data)) {
 		xfer_bytes = data->blksz * data->blocks;
 		WARN_ON(xfer_bytes > host->bounce_buf_size);
-		meson_mmc_copy_buffer(host, data, xfer_bytes, false);
+		if (host->dram_access_quirk)
+			meson_mmc_copy_buffer(host, data, xfer_bytes, false);
+		else
+			sg_copy_from_buffer(data->sg, data->sg_len,
+					    host->bounce_buf, xfer_bytes);
 	}
 
 	next_cmd = meson_mmc_get_next_command(cmd);
-- 
2.25.1

[PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Neil Armstrong]

The memory at the end of the controller only accepts 32bit read/write
accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
(which will be split into two 32bit access) and 8bit leading to incomplete
copies to/from this memory when the buffer is not multiple of 8bytes.

Add a local copy using writel/readl accesses to make sure we use the right
memory access width.

The switch to memcpy_to/fromio was done because of 285133040e6c
("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
worked before since it mainly used 32bit memory acceses.

Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
Reported-by: Christian Hewitt <christianshewitt@gmail.com>
Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 3f28eb4d17fe..08c0ff0bfa8b 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
 	writel(start, host->regs + SD_EMMC_START);
 }
 
-/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
+/* local sg copy for dram_access_quirk */
 static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
 				  size_t buflen, bool to_buffer)
 {
@@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
 	sg_miter_start(&miter, sgl, nents, sg_flags);
 
 	while ((offset < buflen) && sg_miter_next(&miter)) {
-		unsigned int len;
+		unsigned int buf_offset = 0;
+		unsigned int len, left;
+		u32 *buf = miter.addr;
+
+		if (((unsigned long int)miter.addr % 4))
+			dev_err(host->dev, "non word aligned sg");
 
 		len = min(miter.length, buflen - offset);
 
-		/* When dram_access_quirk, the bounce buffer is a iomem mapping */
-		if (host->dram_access_quirk) {
-			if (to_buffer)
-				memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
-			else
-				memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
+		if ((len % 4))
+			dev_err(host->dev, "non word multiple sg");
+
+		left = len;
+
+		if (to_buffer) {
+			do {
+				writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
+
+				buf_offset += 4;
+				left -= 4;
+			} while (left);
 		} else {
-			if (to_buffer)
-				memcpy(host->bounce_buf + offset, miter.addr, len);
-			else
-				memcpy(miter.addr, host->bounce_buf + offset, len);
+			do {
+				*buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
+
+				buf_offset += 4;
+				left -= 4;
+			} while (left);
 		}
 
 		offset += len;
@@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 		if (data->flags & MMC_DATA_WRITE) {
 			cmd_cfg |= CMD_CFG_DATA_WR;
 			WARN_ON(xfer_bytes > host->bounce_buf_size);
-			meson_mmc_copy_buffer(host, data, xfer_bytes, true);
+			if (host->dram_access_quirk)
+				meson_mmc_copy_buffer(host, data, xfer_bytes, true);
+			else
+				sg_copy_to_buffer(data->sg, data->sg_len,
+						  host->bounce_buf, xfer_bytes);
 			dma_wmb();
 		}
 
@@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 	if (meson_mmc_bounce_buf_read(data)) {
 		xfer_bytes = data->blksz * data->blocks;
 		WARN_ON(xfer_bytes > host->bounce_buf_size);
-		meson_mmc_copy_buffer(host, data, xfer_bytes, false);
+		if (host->dram_access_quirk)
+			meson_mmc_copy_buffer(host, data, xfer_bytes, false);
+		else
+			sg_copy_from_buffer(data->sg, data->sg_len,
+					    host->bounce_buf, xfer_bytes);
 	}
 
 	next_cmd = meson_mmc_get_next_command(cmd);
-- 
2.25.1


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

[PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Neil Armstrong]

The memory at the end of the controller only accepts 32bit read/write
accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
(which will be split into two 32bit access) and 8bit leading to incomplete
copies to/from this memory when the buffer is not multiple of 8bytes.

Add a local copy using writel/readl accesses to make sure we use the right
memory access width.

The switch to memcpy_to/fromio was done because of 285133040e6c
("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
worked before since it mainly used 32bit memory acceses.

Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
Reported-by: Christian Hewitt <christianshewitt@gmail.com>
Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 3f28eb4d17fe..08c0ff0bfa8b 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
 	writel(start, host->regs + SD_EMMC_START);
 }
 
-/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
+/* local sg copy for dram_access_quirk */
 static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
 				  size_t buflen, bool to_buffer)
 {
@@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
 	sg_miter_start(&miter, sgl, nents, sg_flags);
 
 	while ((offset < buflen) && sg_miter_next(&miter)) {
-		unsigned int len;
+		unsigned int buf_offset = 0;
+		unsigned int len, left;
+		u32 *buf = miter.addr;
+
+		if (((unsigned long int)miter.addr % 4))
+			dev_err(host->dev, "non word aligned sg");
 
 		len = min(miter.length, buflen - offset);
 
-		/* When dram_access_quirk, the bounce buffer is a iomem mapping */
-		if (host->dram_access_quirk) {
-			if (to_buffer)
-				memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
-			else
-				memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
+		if ((len % 4))
+			dev_err(host->dev, "non word multiple sg");
+
+		left = len;
+
+		if (to_buffer) {
+			do {
+				writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
+
+				buf_offset += 4;
+				left -= 4;
+			} while (left);
 		} else {
-			if (to_buffer)
-				memcpy(host->bounce_buf + offset, miter.addr, len);
-			else
-				memcpy(miter.addr, host->bounce_buf + offset, len);
+			do {
+				*buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
+
+				buf_offset += 4;
+				left -= 4;
+			} while (left);
 		}
 
 		offset += len;
@@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 		if (data->flags & MMC_DATA_WRITE) {
 			cmd_cfg |= CMD_CFG_DATA_WR;
 			WARN_ON(xfer_bytes > host->bounce_buf_size);
-			meson_mmc_copy_buffer(host, data, xfer_bytes, true);
+			if (host->dram_access_quirk)
+				meson_mmc_copy_buffer(host, data, xfer_bytes, true);
+			else
+				sg_copy_to_buffer(data->sg, data->sg_len,
+						  host->bounce_buf, xfer_bytes);
 			dma_wmb();
 		}
 
@@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 	if (meson_mmc_bounce_buf_read(data)) {
 		xfer_bytes = data->blksz * data->blocks;
 		WARN_ON(xfer_bytes > host->bounce_buf_size);
-		meson_mmc_copy_buffer(host, data, xfer_bytes, false);
+		if (host->dram_access_quirk)
+			meson_mmc_copy_buffer(host, data, xfer_bytes, false);
+		else
+			sg_copy_from_buffer(data->sg, data->sg_len,
+					    host->bounce_buf, xfer_bytes);
 	}
 
 	next_cmd = meson_mmc_get_next_command(cmd);
-- 
2.25.1


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

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Martin Blumenstingl]

On Mon, Sep 13, 2021 at 10:05 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The memory at the end of the controller only accepts 32bit read/write
> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
> (which will be split into two 32bit access) and 8bit leading to incomplete
> copies to/from this memory when the buffer is not multiple of 8bytes.
>
> Add a local copy using writel/readl accesses to make sure we use the right
> memory access width.
>
> The switch to memcpy_to/fromio was done because of 285133040e6c
> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
> worked before since it mainly used 32bit memory acceses.
>
> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Martin Blumenstingl]

On Mon, Sep 13, 2021 at 10:05 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The memory at the end of the controller only accepts 32bit read/write
> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
> (which will be split into two 32bit access) and 8bit leading to incomplete
> copies to/from this memory when the buffer is not multiple of 8bytes.
>
> Add a local copy using writel/readl accesses to make sure we use the right
> memory access width.
>
> The switch to memcpy_to/fromio was done because of 285133040e6c
> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
> worked before since it mainly used 32bit memory acceses.
>
> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Martin Blumenstingl]

On Mon, Sep 13, 2021 at 10:05 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The memory at the end of the controller only accepts 32bit read/write
> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
> (which will be split into two 32bit access) and 8bit leading to incomplete
> copies to/from this memory when the buffer is not multiple of 8bytes.
>
> Add a local copy using writel/readl accesses to make sure we use the right
> memory access width.
>
> The switch to memcpy_to/fromio was done because of 285133040e6c
> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
> worked before since it mainly used 32bit memory acceses.
>
> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Ulf Hansson]

On Mon, 13 Sept 2021 at 10:05, Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The memory at the end of the controller only accepts 32bit read/write
> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
> (which will be split into two 32bit access) and 8bit leading to incomplete
> copies to/from this memory when the buffer is not multiple of 8bytes.
>
> Add a local copy using writel/readl accesses to make sure we use the right
> memory access width.
>
> The switch to memcpy_to/fromio was done because of 285133040e6c
> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
> worked before since it mainly used 32bit memory acceses.
>
> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 3f28eb4d17fe..08c0ff0bfa8b 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
>         writel(start, host->regs + SD_EMMC_START);
>  }
>
> -/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
> +/* local sg copy for dram_access_quirk */
>  static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
>                                   size_t buflen, bool to_buffer)
>  {
> @@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
>         sg_miter_start(&miter, sgl, nents, sg_flags);
>
>         while ((offset < buflen) && sg_miter_next(&miter)) {
> -               unsigned int len;
> +               unsigned int buf_offset = 0;
> +               unsigned int len, left;
> +               u32 *buf = miter.addr;
> +
> +               if (((unsigned long int)miter.addr % 4))
> +                       dev_err(host->dev, "non word aligned sg");

This looks weird. You print an error message, but continue to process
data? If this is a case you can't handle, perhaps you should propagate
an error code instead?

Additionally, you may want to use the IS_ALIGNED() macro.

>
>                 len = min(miter.length, buflen - offset);
>
> -               /* When dram_access_quirk, the bounce buffer is a iomem mapping */
> -               if (host->dram_access_quirk) {
> -                       if (to_buffer)
> -                               memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
> -                       else
> -                               memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
> +               if ((len % 4))
> +                       dev_err(host->dev, "non word multiple sg");

Again, a dev_err() doesn't seem like the right thing to do. If you
can't handle this, please return an error code instead.

Perhaps returning an error code isn't convenient at this point. An
option could then be to pre-validate the sglist at the time of
starting the request. We have other host drivers doing this, have a
look at drivers/mmc/host/mmci*, for example.

> +
> +               left = len;
> +
> +               if (to_buffer) {
> +                       do {
> +                               writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
> +
> +                               buf_offset += 4;
> +                               left -= 4;
> +                       } while (left);
>                 } else {
> -                       if (to_buffer)
> -                               memcpy(host->bounce_buf + offset, miter.addr, len);
> -                       else
> -                               memcpy(miter.addr, host->bounce_buf + offset, len);
> +                       do {
> +                               *buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
> +
> +                               buf_offset += 4;
> +                               left -= 4;
> +                       } while (left);
>                 }
>
>                 offset += len;
> @@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>                 if (data->flags & MMC_DATA_WRITE) {
>                         cmd_cfg |= CMD_CFG_DATA_WR;
>                         WARN_ON(xfer_bytes > host->bounce_buf_size);
> -                       meson_mmc_copy_buffer(host, data, xfer_bytes, true);
> +                       if (host->dram_access_quirk)
> +                               meson_mmc_copy_buffer(host, data, xfer_bytes, true);
> +                       else
> +                               sg_copy_to_buffer(data->sg, data->sg_len,
> +                                                 host->bounce_buf, xfer_bytes);
>                         dma_wmb();
>                 }
>
> @@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>         if (meson_mmc_bounce_buf_read(data)) {
>                 xfer_bytes = data->blksz * data->blocks;
>                 WARN_ON(xfer_bytes > host->bounce_buf_size);
> -               meson_mmc_copy_buffer(host, data, xfer_bytes, false);
> +               if (host->dram_access_quirk)
> +                       meson_mmc_copy_buffer(host, data, xfer_bytes, false);
> +               else
> +                       sg_copy_from_buffer(data->sg, data->sg_len,
> +                                           host->bounce_buf, xfer_bytes);
>         }
>
>         next_cmd = meson_mmc_get_next_command(cmd);
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Ulf Hansson]

On Mon, 13 Sept 2021 at 10:05, Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The memory at the end of the controller only accepts 32bit read/write
> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
> (which will be split into two 32bit access) and 8bit leading to incomplete
> copies to/from this memory when the buffer is not multiple of 8bytes.
>
> Add a local copy using writel/readl accesses to make sure we use the right
> memory access width.
>
> The switch to memcpy_to/fromio was done because of 285133040e6c
> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
> worked before since it mainly used 32bit memory acceses.
>
> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 3f28eb4d17fe..08c0ff0bfa8b 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
>         writel(start, host->regs + SD_EMMC_START);
>  }
>
> -/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
> +/* local sg copy for dram_access_quirk */
>  static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
>                                   size_t buflen, bool to_buffer)
>  {
> @@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
>         sg_miter_start(&miter, sgl, nents, sg_flags);
>
>         while ((offset < buflen) && sg_miter_next(&miter)) {
> -               unsigned int len;
> +               unsigned int buf_offset = 0;
> +               unsigned int len, left;
> +               u32 *buf = miter.addr;
> +
> +               if (((unsigned long int)miter.addr % 4))
> +                       dev_err(host->dev, "non word aligned sg");

This looks weird. You print an error message, but continue to process
data? If this is a case you can't handle, perhaps you should propagate
an error code instead?

Additionally, you may want to use the IS_ALIGNED() macro.

>
>                 len = min(miter.length, buflen - offset);
>
> -               /* When dram_access_quirk, the bounce buffer is a iomem mapping */
> -               if (host->dram_access_quirk) {
> -                       if (to_buffer)
> -                               memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
> -                       else
> -                               memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
> +               if ((len % 4))
> +                       dev_err(host->dev, "non word multiple sg");

Again, a dev_err() doesn't seem like the right thing to do. If you
can't handle this, please return an error code instead.

Perhaps returning an error code isn't convenient at this point. An
option could then be to pre-validate the sglist at the time of
starting the request. We have other host drivers doing this, have a
look at drivers/mmc/host/mmci*, for example.

> +
> +               left = len;
> +
> +               if (to_buffer) {
> +                       do {
> +                               writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
> +
> +                               buf_offset += 4;
> +                               left -= 4;
> +                       } while (left);
>                 } else {
> -                       if (to_buffer)
> -                               memcpy(host->bounce_buf + offset, miter.addr, len);
> -                       else
> -                               memcpy(miter.addr, host->bounce_buf + offset, len);
> +                       do {
> +                               *buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
> +
> +                               buf_offset += 4;
> +                               left -= 4;
> +                       } while (left);
>                 }
>
>                 offset += len;
> @@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>                 if (data->flags & MMC_DATA_WRITE) {
>                         cmd_cfg |= CMD_CFG_DATA_WR;
>                         WARN_ON(xfer_bytes > host->bounce_buf_size);
> -                       meson_mmc_copy_buffer(host, data, xfer_bytes, true);
> +                       if (host->dram_access_quirk)
> +                               meson_mmc_copy_buffer(host, data, xfer_bytes, true);
> +                       else
> +                               sg_copy_to_buffer(data->sg, data->sg_len,
> +                                                 host->bounce_buf, xfer_bytes);
>                         dma_wmb();
>                 }
>
> @@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>         if (meson_mmc_bounce_buf_read(data)) {
>                 xfer_bytes = data->blksz * data->blocks;
>                 WARN_ON(xfer_bytes > host->bounce_buf_size);
> -               meson_mmc_copy_buffer(host, data, xfer_bytes, false);
> +               if (host->dram_access_quirk)
> +                       meson_mmc_copy_buffer(host, data, xfer_bytes, false);
> +               else
> +                       sg_copy_from_buffer(data->sg, data->sg_len,
> +                                           host->bounce_buf, xfer_bytes);
>         }
>
>         next_cmd = meson_mmc_get_next_command(cmd);
> --
> 2.25.1
>

Kind regards
Uffe

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

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Ulf Hansson]

On Mon, 13 Sept 2021 at 10:05, Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The memory at the end of the controller only accepts 32bit read/write
> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
> (which will be split into two 32bit access) and 8bit leading to incomplete
> copies to/from this memory when the buffer is not multiple of 8bytes.
>
> Add a local copy using writel/readl accesses to make sure we use the right
> memory access width.
>
> The switch to memcpy_to/fromio was done because of 285133040e6c
> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
> worked before since it mainly used 32bit memory acceses.
>
> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 3f28eb4d17fe..08c0ff0bfa8b 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
>         writel(start, host->regs + SD_EMMC_START);
>  }
>
> -/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
> +/* local sg copy for dram_access_quirk */
>  static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
>                                   size_t buflen, bool to_buffer)
>  {
> @@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
>         sg_miter_start(&miter, sgl, nents, sg_flags);
>
>         while ((offset < buflen) && sg_miter_next(&miter)) {
> -               unsigned int len;
> +               unsigned int buf_offset = 0;
> +               unsigned int len, left;
> +               u32 *buf = miter.addr;
> +
> +               if (((unsigned long int)miter.addr % 4))
> +                       dev_err(host->dev, "non word aligned sg");

This looks weird. You print an error message, but continue to process
data? If this is a case you can't handle, perhaps you should propagate
an error code instead?

Additionally, you may want to use the IS_ALIGNED() macro.

>
>                 len = min(miter.length, buflen - offset);
>
> -               /* When dram_access_quirk, the bounce buffer is a iomem mapping */
> -               if (host->dram_access_quirk) {
> -                       if (to_buffer)
> -                               memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
> -                       else
> -                               memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
> +               if ((len % 4))
> +                       dev_err(host->dev, "non word multiple sg");

Again, a dev_err() doesn't seem like the right thing to do. If you
can't handle this, please return an error code instead.

Perhaps returning an error code isn't convenient at this point. An
option could then be to pre-validate the sglist at the time of
starting the request. We have other host drivers doing this, have a
look at drivers/mmc/host/mmci*, for example.

> +
> +               left = len;
> +
> +               if (to_buffer) {
> +                       do {
> +                               writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
> +
> +                               buf_offset += 4;
> +                               left -= 4;
> +                       } while (left);
>                 } else {
> -                       if (to_buffer)
> -                               memcpy(host->bounce_buf + offset, miter.addr, len);
> -                       else
> -                               memcpy(miter.addr, host->bounce_buf + offset, len);
> +                       do {
> +                               *buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
> +
> +                               buf_offset += 4;
> +                               left -= 4;
> +                       } while (left);
>                 }
>
>                 offset += len;
> @@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>                 if (data->flags & MMC_DATA_WRITE) {
>                         cmd_cfg |= CMD_CFG_DATA_WR;
>                         WARN_ON(xfer_bytes > host->bounce_buf_size);
> -                       meson_mmc_copy_buffer(host, data, xfer_bytes, true);
> +                       if (host->dram_access_quirk)
> +                               meson_mmc_copy_buffer(host, data, xfer_bytes, true);
> +                       else
> +                               sg_copy_to_buffer(data->sg, data->sg_len,
> +                                                 host->bounce_buf, xfer_bytes);
>                         dma_wmb();
>                 }
>
> @@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>         if (meson_mmc_bounce_buf_read(data)) {
>                 xfer_bytes = data->blksz * data->blocks;
>                 WARN_ON(xfer_bytes > host->bounce_buf_size);
> -               meson_mmc_copy_buffer(host, data, xfer_bytes, false);
> +               if (host->dram_access_quirk)
> +                       meson_mmc_copy_buffer(host, data, xfer_bytes, false);
> +               else
> +                       sg_copy_from_buffer(data->sg, data->sg_len,
> +                                           host->bounce_buf, xfer_bytes);
>         }
>
>         next_cmd = meson_mmc_get_next_command(cmd);
> --
> 2.25.1
>

Kind regards
Uffe

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

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Neil Armstrong]

Hi,

On 23/09/2021 12:51, Ulf Hansson wrote:
> On Mon, 13 Sept 2021 at 10:05, Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> The memory at the end of the controller only accepts 32bit read/write
>> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
>> (which will be split into two 32bit access) and 8bit leading to incomplete
>> copies to/from this memory when the buffer is not multiple of 8bytes.
>>
>> Add a local copy using writel/readl accesses to make sure we use the right
>> memory access width.
>>
>> The switch to memcpy_to/fromio was done because of 285133040e6c
>> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
>> worked before since it mainly used 32bit memory acceses.
>>
>> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
>> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
>> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
>>  1 file changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>> index 3f28eb4d17fe..08c0ff0bfa8b 100644
>> --- a/drivers/mmc/host/meson-gx-mmc.c
>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>> @@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
>>         writel(start, host->regs + SD_EMMC_START);
>>  }
>>
>> -/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
>> +/* local sg copy for dram_access_quirk */
>>  static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
>>                                   size_t buflen, bool to_buffer)
>>  {
>> @@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
>>         sg_miter_start(&miter, sgl, nents, sg_flags);
>>
>>         while ((offset < buflen) && sg_miter_next(&miter)) {
>> -               unsigned int len;
>> +               unsigned int buf_offset = 0;
>> +               unsigned int len, left;
>> +               u32 *buf = miter.addr;
>> +
>> +               if (((unsigned long int)miter.addr % 4))
>> +                       dev_err(host->dev, "non word aligned sg");
> 
> This looks weird. You print an error message, but continue to process
> data? If this is a case you can't handle, perhaps you should propagate
> an error code instead?
> 
> Additionally, you may want to use the IS_ALIGNED() macro.
> 
>>
>>                 len = min(miter.length, buflen - offset);
>>
>> -               /* When dram_access_quirk, the bounce buffer is a iomem mapping */
>> -               if (host->dram_access_quirk) {
>> -                       if (to_buffer)
>> -                               memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
>> -                       else
>> -                               memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
>> +               if ((len % 4))
>> +                       dev_err(host->dev, "non word multiple sg");
> 
> Again, a dev_err() doesn't seem like the right thing to do. If you
> can't handle this, please return an error code instead.
> 
> Perhaps returning an error code isn't convenient at this point. An
> option could then be to pre-validate the sglist at the time of
> starting the request. We have other host drivers doing this, have a
> look at drivers/mmc/host/mmci*, for example.

Yep pre-validating the data at the request callback seems the best solution,

Thanks,
Neil

> 
>> +
>> +               left = len;
>> +
>> +               if (to_buffer) {
>> +                       do {
>> +                               writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
>> +
>> +                               buf_offset += 4;
>> +                               left -= 4;
>> +                       } while (left);
>>                 } else {
>> -                       if (to_buffer)
>> -                               memcpy(host->bounce_buf + offset, miter.addr, len);
>> -                       else
>> -                               memcpy(miter.addr, host->bounce_buf + offset, len);
>> +                       do {
>> +                               *buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
>> +
>> +                               buf_offset += 4;
>> +                               left -= 4;
>> +                       } while (left);
>>                 }
>>
>>                 offset += len;
>> @@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>>                 if (data->flags & MMC_DATA_WRITE) {
>>                         cmd_cfg |= CMD_CFG_DATA_WR;
>>                         WARN_ON(xfer_bytes > host->bounce_buf_size);
>> -                       meson_mmc_copy_buffer(host, data, xfer_bytes, true);
>> +                       if (host->dram_access_quirk)
>> +                               meson_mmc_copy_buffer(host, data, xfer_bytes, true);
>> +                       else
>> +                               sg_copy_to_buffer(data->sg, data->sg_len,
>> +                                                 host->bounce_buf, xfer_bytes);
>>                         dma_wmb();
>>                 }
>>
>> @@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>>         if (meson_mmc_bounce_buf_read(data)) {
>>                 xfer_bytes = data->blksz * data->blocks;
>>                 WARN_ON(xfer_bytes > host->bounce_buf_size);
>> -               meson_mmc_copy_buffer(host, data, xfer_bytes, false);
>> +               if (host->dram_access_quirk)
>> +                       meson_mmc_copy_buffer(host, data, xfer_bytes, false);
>> +               else
>> +                       sg_copy_from_buffer(data->sg, data->sg_len,
>> +                                           host->bounce_buf, xfer_bytes);
>>         }
>>
>>         next_cmd = meson_mmc_get_next_command(cmd);
>> --
>> 2.25.1
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Neil Armstrong]

Hi,

On 23/09/2021 12:51, Ulf Hansson wrote:
> On Mon, 13 Sept 2021 at 10:05, Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> The memory at the end of the controller only accepts 32bit read/write
>> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
>> (which will be split into two 32bit access) and 8bit leading to incomplete
>> copies to/from this memory when the buffer is not multiple of 8bytes.
>>
>> Add a local copy using writel/readl accesses to make sure we use the right
>> memory access width.
>>
>> The switch to memcpy_to/fromio was done because of 285133040e6c
>> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
>> worked before since it mainly used 32bit memory acceses.
>>
>> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
>> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
>> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
>>  1 file changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>> index 3f28eb4d17fe..08c0ff0bfa8b 100644
>> --- a/drivers/mmc/host/meson-gx-mmc.c
>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>> @@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
>>         writel(start, host->regs + SD_EMMC_START);
>>  }
>>
>> -/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
>> +/* local sg copy for dram_access_quirk */
>>  static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
>>                                   size_t buflen, bool to_buffer)
>>  {
>> @@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
>>         sg_miter_start(&miter, sgl, nents, sg_flags);
>>
>>         while ((offset < buflen) && sg_miter_next(&miter)) {
>> -               unsigned int len;
>> +               unsigned int buf_offset = 0;
>> +               unsigned int len, left;
>> +               u32 *buf = miter.addr;
>> +
>> +               if (((unsigned long int)miter.addr % 4))
>> +                       dev_err(host->dev, "non word aligned sg");
> 
> This looks weird. You print an error message, but continue to process
> data? If this is a case you can't handle, perhaps you should propagate
> an error code instead?
> 
> Additionally, you may want to use the IS_ALIGNED() macro.
> 
>>
>>                 len = min(miter.length, buflen - offset);
>>
>> -               /* When dram_access_quirk, the bounce buffer is a iomem mapping */
>> -               if (host->dram_access_quirk) {
>> -                       if (to_buffer)
>> -                               memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
>> -                       else
>> -                               memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
>> +               if ((len % 4))
>> +                       dev_err(host->dev, "non word multiple sg");
> 
> Again, a dev_err() doesn't seem like the right thing to do. If you
> can't handle this, please return an error code instead.
> 
> Perhaps returning an error code isn't convenient at this point. An
> option could then be to pre-validate the sglist at the time of
> starting the request. We have other host drivers doing this, have a
> look at drivers/mmc/host/mmci*, for example.

Yep pre-validating the data at the request callback seems the best solution,

Thanks,
Neil

> 
>> +
>> +               left = len;
>> +
>> +               if (to_buffer) {
>> +                       do {
>> +                               writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
>> +
>> +                               buf_offset += 4;
>> +                               left -= 4;
>> +                       } while (left);
>>                 } else {
>> -                       if (to_buffer)
>> -                               memcpy(host->bounce_buf + offset, miter.addr, len);
>> -                       else
>> -                               memcpy(miter.addr, host->bounce_buf + offset, len);
>> +                       do {
>> +                               *buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
>> +
>> +                               buf_offset += 4;
>> +                               left -= 4;
>> +                       } while (left);
>>                 }
>>
>>                 offset += len;
>> @@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>>                 if (data->flags & MMC_DATA_WRITE) {
>>                         cmd_cfg |= CMD_CFG_DATA_WR;
>>                         WARN_ON(xfer_bytes > host->bounce_buf_size);
>> -                       meson_mmc_copy_buffer(host, data, xfer_bytes, true);
>> +                       if (host->dram_access_quirk)
>> +                               meson_mmc_copy_buffer(host, data, xfer_bytes, true);
>> +                       else
>> +                               sg_copy_to_buffer(data->sg, data->sg_len,
>> +                                                 host->bounce_buf, xfer_bytes);
>>                         dma_wmb();
>>                 }
>>
>> @@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>>         if (meson_mmc_bounce_buf_read(data)) {
>>                 xfer_bytes = data->blksz * data->blocks;
>>                 WARN_ON(xfer_bytes > host->bounce_buf_size);
>> -               meson_mmc_copy_buffer(host, data, xfer_bytes, false);
>> +               if (host->dram_access_quirk)
>> +                       meson_mmc_copy_buffer(host, data, xfer_bytes, false);
>> +               else
>> +                       sg_copy_from_buffer(data->sg, data->sg_len,
>> +                                           host->bounce_buf, xfer_bytes);
>>         }
>>
>>         next_cmd = meson_mmc_get_next_command(cmd);
>> --
>> 2.25.1
>>
> 
> Kind regards
> Uffe
> 


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

Re: [PATCH] mmc: meson-gx: do not use memcpy_to/fromio for dram-access-quirk

[Neil Armstrong]

Hi,

On 23/09/2021 12:51, Ulf Hansson wrote:
> On Mon, 13 Sept 2021 at 10:05, Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> The memory at the end of the controller only accepts 32bit read/write
>> accesses, but the arm64 memcpy_to/fromio implementation only uses 64bit
>> (which will be split into two 32bit access) and 8bit leading to incomplete
>> copies to/from this memory when the buffer is not multiple of 8bytes.
>>
>> Add a local copy using writel/readl accesses to make sure we use the right
>> memory access width.
>>
>> The switch to memcpy_to/fromio was done because of 285133040e6c
>> ("arm64: Import latest memcpy()/memmove() implementation"), but using memcpy
>> worked before since it mainly used 32bit memory acceses.
>>
>> Fixes: 103a5348c22c ("mmc: meson-gx: use memcpy_to/fromio for dram-access-quirk")
>> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
>> Suggested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/mmc/host/meson-gx-mmc.c | 49 +++++++++++++++++++++++----------
>>  1 file changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>> index 3f28eb4d17fe..08c0ff0bfa8b 100644
>> --- a/drivers/mmc/host/meson-gx-mmc.c
>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>> @@ -746,7 +746,7 @@ static void meson_mmc_desc_chain_transfer(struct mmc_host *mmc, u32 cmd_cfg)
>>         writel(start, host->regs + SD_EMMC_START);
>>  }
>>
>> -/* local sg copy to buffer version with _to/fromio usage for dram_access_quirk */
>> +/* local sg copy for dram_access_quirk */
>>  static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data,
>>                                   size_t buflen, bool to_buffer)
>>  {
>> @@ -764,21 +764,34 @@ static void meson_mmc_copy_buffer(struct meson_host *host, struct mmc_data *data
>>         sg_miter_start(&miter, sgl, nents, sg_flags);
>>
>>         while ((offset < buflen) && sg_miter_next(&miter)) {
>> -               unsigned int len;
>> +               unsigned int buf_offset = 0;
>> +               unsigned int len, left;
>> +               u32 *buf = miter.addr;
>> +
>> +               if (((unsigned long int)miter.addr % 4))
>> +                       dev_err(host->dev, "non word aligned sg");
> 
> This looks weird. You print an error message, but continue to process
> data? If this is a case you can't handle, perhaps you should propagate
> an error code instead?
> 
> Additionally, you may want to use the IS_ALIGNED() macro.
> 
>>
>>                 len = min(miter.length, buflen - offset);
>>
>> -               /* When dram_access_quirk, the bounce buffer is a iomem mapping */
>> -               if (host->dram_access_quirk) {
>> -                       if (to_buffer)
>> -                               memcpy_toio(host->bounce_iomem_buf + offset, miter.addr, len);
>> -                       else
>> -                               memcpy_fromio(miter.addr, host->bounce_iomem_buf + offset, len);
>> +               if ((len % 4))
>> +                       dev_err(host->dev, "non word multiple sg");
> 
> Again, a dev_err() doesn't seem like the right thing to do. If you
> can't handle this, please return an error code instead.
> 
> Perhaps returning an error code isn't convenient at this point. An
> option could then be to pre-validate the sglist at the time of
> starting the request. We have other host drivers doing this, have a
> look at drivers/mmc/host/mmci*, for example.

Yep pre-validating the data at the request callback seems the best solution,

Thanks,
Neil

> 
>> +
>> +               left = len;
>> +
>> +               if (to_buffer) {
>> +                       do {
>> +                               writel(*buf++, host->bounce_iomem_buf + offset + buf_offset);
>> +
>> +                               buf_offset += 4;
>> +                               left -= 4;
>> +                       } while (left);
>>                 } else {
>> -                       if (to_buffer)
>> -                               memcpy(host->bounce_buf + offset, miter.addr, len);
>> -                       else
>> -                               memcpy(miter.addr, host->bounce_buf + offset, len);
>> +                       do {
>> +                               *buf++ = readl(host->bounce_iomem_buf + offset + buf_offset);
>> +
>> +                               buf_offset += 4;
>> +                               left -= 4;
>> +                       } while (left);
>>                 }
>>
>>                 offset += len;
>> @@ -830,7 +843,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>>                 if (data->flags & MMC_DATA_WRITE) {
>>                         cmd_cfg |= CMD_CFG_DATA_WR;
>>                         WARN_ON(xfer_bytes > host->bounce_buf_size);
>> -                       meson_mmc_copy_buffer(host, data, xfer_bytes, true);
>> +                       if (host->dram_access_quirk)
>> +                               meson_mmc_copy_buffer(host, data, xfer_bytes, true);
>> +                       else
>> +                               sg_copy_to_buffer(data->sg, data->sg_len,
>> +                                                 host->bounce_buf, xfer_bytes);
>>                         dma_wmb();
>>                 }
>>
>> @@ -999,7 +1016,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>>         if (meson_mmc_bounce_buf_read(data)) {
>>                 xfer_bytes = data->blksz * data->blocks;
>>                 WARN_ON(xfer_bytes > host->bounce_buf_size);
>> -               meson_mmc_copy_buffer(host, data, xfer_bytes, false);
>> +               if (host->dram_access_quirk)
>> +                       meson_mmc_copy_buffer(host, data, xfer_bytes, false);
>> +               else
>> +                       sg_copy_from_buffer(data->sg, data->sg_len,
>> +                                           host->bounce_buf, xfer_bytes);
>>         }
>>
>>         next_cmd = meson_mmc_get_next_command(cmd);
>> --
>> 2.25.1
>>
> 
> Kind regards
> Uffe
> 


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