[RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Wolfram Sang]

This patch fixes 2 problems:
[1] The output warning logs and data loss when performing
mount/umount then remount the device with jffs2 format.
[2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.

This is the sample warning logs when performing mount/umount then
remount the device with jffs2 format:
jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
Read 0x00034e00, calculated 0xadb272a7

The reason for issue [1] is that the writing data seems to
get messed up.
Data is only completed when the number of bytes is divisible by 4.
If you only have 3 bytes of data left to write, 1 garbage byte
is inserted after the end of the write stream.
If you only have 2 bytes of data left to write, 2 bytes of '00'
are added into the write stream.
If you only have 1 byte of data left to write, 2 bytes of '00'
are added into the write stream. 1 garbage byte is inserted after
the end of the write stream.

To solve problem [1], data must be written continuously in serial
and the write stream ends when data is out.

Following HW manual 62.2.15, access to SMWDR0 register should be
in the same size as the transfer size specified in the SPIDE[3:0]
bits in the manual mode enable setting register (SMENR).
Be sure to access from address 0.

So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
accessed by 16-bit width.
Similar to SMWDR1, SMDDR0/1 registers.
In current code, SMWDR0 register is accessed by regmap_write()
that only set up to do 32-bit width.

To solve problem [2], data must be written 16-bit or 8-bit when
transferring 1-byte or 2-byte.

Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
[wsa: refactored to use regmap only via reg_read/reg_write]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Hi,

I could reproduce the issue by a simple:

  $ echo "Hello" > /dev/mtd10

The original BSP patch fixed the issue but mixed regmap-acces with
ioread/iowrite accesses. So, I refactored it to use custom regmap
accessors. This keeps the code more readable IMO. With this patch, my
custom test cases work as well as the JFFS2 remount mentioned in the
commit message. Tested on a Renesas Condor board (R-Car V3M) and a
Falcon board (R-Car V3U). I send this as RFC because this is my first
patch for the RPC code and hope for feedback. The BSP team has been
contacted as well for comments and testing. Nonetheless, this addresses
a serious issue which has caused broken boards because of writing to
unintended locations. So, I'd like to see this discussed and applied
soon if possible.

Thanks everyone,

   Wolfram


 drivers/memory/renesas-rpc-if.c | 113 ++++++++++++++++++++++----------
 include/memory/renesas-rpc-if.h |   1 +
 2 files changed, 79 insertions(+), 35 deletions(-)

diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index 45eed659b0c6..77a011d5ff8c 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -160,10 +160,62 @@ static const struct regmap_access_table rpcif_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(rpcif_volatile_ranges),
 };
 
+
+/*
+ * Custom accessor functions to ensure SMRDR0 and SMWDR0 are always accessed
+ * with proper width. Requires SMENR_SPIDE to be correctly set before!
+ */
+static int rpcif_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct rpcif *rpc = context;
+
+	if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
+		u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
+
+		if (spide == 0x8) {
+			*val = readb(rpc->base + reg);
+			return 0;
+		} else if (spide == 0xC) {
+			*val = readw(rpc->base + reg);
+			return 0;
+		} else if (spide != 0xF) {
+			return -EILSEQ;
+		}
+	}
+
+	*val = readl(rpc->base + reg);
+	return 0;
+
+}
+
+static int rpcif_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct rpcif *rpc = context;
+
+	if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
+		u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
+
+		if (spide == 0x8) {
+			writeb(val, rpc->base + reg);
+			return 0;
+		} else if (spide == 0xC) {
+			writew(val, rpc->base + reg);
+			return 0;
+		} else if (spide != 0xF) {
+			return -EILSEQ;
+		}
+	}
+
+	writel(val, rpc->base + reg);
+	return 0;
+}
+
 static const struct regmap_config rpcif_regmap_config = {
 	.reg_bits	= 32,
 	.val_bits	= 32,
 	.reg_stride	= 4,
+	.reg_read	= rpcif_reg_read,
+	.reg_write	= rpcif_reg_write,
 	.fast_io	= true,
 	.max_register	= RPCIF_PHYINT,
 	.volatile_table	= &rpcif_volatile_table,
@@ -173,17 +225,15 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct resource *res;
-	void __iomem *base;
 
 	rpc->dev = dev;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	rpc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rpc->base))
+		return PTR_ERR(rpc->base);
 
-	rpc->regmap = devm_regmap_init_mmio(&pdev->dev, base,
-					    &rpcif_regmap_config);
+	rpc->regmap = devm_regmap_init(&pdev->dev, NULL, rpc, &rpcif_regmap_config);
 	if (IS_ERR(rpc->regmap)) {
 		dev_err(&pdev->dev,
 			"failed to init regmap for rpcif, error %ld\n",
@@ -354,20 +404,16 @@ void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
 			nbytes = op->data.nbytes;
 		rpc->xferlen = nbytes;
 
-		rpc->enable |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes)) |
-			RPCIF_SMENR_SPIDB(rpcif_bit_size(op->data.buswidth));
+		rpc->enable |= RPCIF_SMENR_SPIDB(rpcif_bit_size(op->data.buswidth));
 	}
 }
 EXPORT_SYMBOL(rpcif_prepare);
 
 int rpcif_manual_xfer(struct rpcif *rpc)
 {
-	u32 smenr, smcr, pos = 0, max = 4;
+	u32 smenr, smcr, pos = 0, max = rpc->bus_size == 2 ? 8 : 4;
 	int ret = 0;
 
-	if (rpc->bus_size == 2)
-		max = 8;
-
 	pm_runtime_get_sync(rpc->dev);
 
 	regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
@@ -378,37 +424,36 @@ int rpcif_manual_xfer(struct rpcif *rpc)
 	regmap_write(rpc->regmap, RPCIF_SMOPR, rpc->option);
 	regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
 	regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
+	regmap_write(rpc->regmap, RPCIF_SMADR, rpc->smadr);
 	smenr = rpc->enable;
 
 	switch (rpc->dir) {
 	case RPCIF_DATA_OUT:
 		while (pos < rpc->xferlen) {
-			u32 nbytes = rpc->xferlen - pos;
-			u32 data[2];
+			u32 bytes_left = rpc->xferlen - pos;
+			u32 nbytes, data[2];
 
 			smcr = rpc->smcr | RPCIF_SMCR_SPIE;
-			if (nbytes > max) {
-				nbytes = max;
+
+			/* nbytes may only be 1, 2, 4, or 8 */
+			nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
+			if (bytes_left > nbytes)
 				smcr |= RPCIF_SMCR_SSLKP;
-			}
+
+			smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
+			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
 
 			memcpy(data, rpc->buffer + pos, nbytes);
-			if (nbytes > 4) {
+			if (nbytes == 8) {
 				regmap_write(rpc->regmap, RPCIF_SMWDR1,
 					     data[0]);
 				regmap_write(rpc->regmap, RPCIF_SMWDR0,
 					     data[1]);
-			} else if (nbytes > 2) {
+			} else {
 				regmap_write(rpc->regmap, RPCIF_SMWDR0,
 					     data[0]);
-			} else	{
-				regmap_write(rpc->regmap, RPCIF_SMWDR0,
-					     data[0] << 16);
 			}
 
-			regmap_write(rpc->regmap, RPCIF_SMADR,
-				     rpc->smadr + pos);
-			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
 			regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
 			ret = wait_msg_xfer_end(rpc);
 			if (ret)
@@ -448,14 +493,16 @@ int rpcif_manual_xfer(struct rpcif *rpc)
 			break;
 		}
 		while (pos < rpc->xferlen) {
-			u32 nbytes = rpc->xferlen - pos;
-			u32 data[2];
+			u32 bytes_left = rpc->xferlen - pos;
+			u32 nbytes, data[2];
 
-			if (nbytes > max)
-				nbytes = max;
+			/* nbytes may only be 1, 2, 4, or 8 */
+			nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
 
 			regmap_write(rpc->regmap, RPCIF_SMADR,
 				     rpc->smadr + pos);
+			smenr &= ~RPCIF_SMENR_SPIDE(0xF);
+			smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
 			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
 			regmap_write(rpc->regmap, RPCIF_SMCR,
 				     rpc->smcr | RPCIF_SMCR_SPIE);
@@ -463,18 +510,14 @@ int rpcif_manual_xfer(struct rpcif *rpc)
 			if (ret)
 				goto err_out;
 
-			if (nbytes > 4) {
+			if (nbytes == 8) {
 				regmap_read(rpc->regmap, RPCIF_SMRDR1,
 					    &data[0]);
 				regmap_read(rpc->regmap, RPCIF_SMRDR0,
 					    &data[1]);
-			} else if (nbytes > 2) {
-				regmap_read(rpc->regmap, RPCIF_SMRDR0,
-					    &data[0]);
-			} else	{
+			} else {
 				regmap_read(rpc->regmap, RPCIF_SMRDR0,
 					    &data[0]);
-				data[0] >>= 16;
 			}
 			memcpy(rpc->buffer + pos, data, nbytes);
 
diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
index e3e770f76f34..77c694a19149 100644
--- a/include/memory/renesas-rpc-if.h
+++ b/include/memory/renesas-rpc-if.h
@@ -59,6 +59,7 @@ struct rpcif_op {
 
 struct rpcif {
 	struct device *dev;
+	void __iomem *base;
 	void __iomem *dirmap;
 	struct regmap *regmap;
 	struct reset_control *rstc;
-- 
2.30.2

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Krzysztof Kozlowski]

On 22/09/2021 11:10, Wolfram Sang wrote:
> This patch fixes 2 problems:
> [1] The output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> 
> This is the sample warning logs when performing mount/umount then
> remount the device with jffs2 format:
> jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> Read 0x00034e00, calculated 0xadb272a7
> 
> The reason for issue [1] is that the writing data seems to
> get messed up.
> Data is only completed when the number of bytes is divisible by 4.
> If you only have 3 bytes of data left to write, 1 garbage byte
> is inserted after the end of the write stream.
> If you only have 2 bytes of data left to write, 2 bytes of '00'
> are added into the write stream.
> If you only have 1 byte of data left to write, 2 bytes of '00'
> are added into the write stream. 1 garbage byte is inserted after
> the end of the write stream.
> 
> To solve problem [1], data must be written continuously in serial
> and the write stream ends when data is out.
> 
> Following HW manual 62.2.15, access to SMWDR0 register should be
> in the same size as the transfer size specified in the SPIDE[3:0]
> bits in the manual mode enable setting register (SMENR).
> Be sure to access from address 0.
> 
> So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
> accessed by 16-bit width.
> Similar to SMWDR1, SMDDR0/1 registers.
> In current code, SMWDR0 register is accessed by regmap_write()
> that only set up to do 32-bit width.

Is this part something worth splitting to its own patch?

> 
> To solve problem [2], data must be written 16-bit or 8-bit when
> transferring 1-byte or 2-byte.
> 
> Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
> [wsa: refactored to use regmap only via reg_read/reg_write]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Hi,
> 
> I could reproduce the issue by a simple:
> 
>   $ echo "Hello" > /dev/mtd10
> 
> The original BSP patch fixed the issue but mixed regmap-acces with
> ioread/iowrite accesses. So, I refactored it to use custom regmap
> accessors. This keeps the code more readable IMO. With this patch, my
> custom test cases work as well as the JFFS2 remount mentioned in the
> commit message. Tested on a Renesas Condor board (R-Car V3M) and a
> Falcon board (R-Car V3U). I send this as RFC because this is my first
> patch for the RPC code and hope for feedback. The BSP team has been
> contacted as well for comments and testing. Nonetheless, this addresses
> a serious issue which has caused broken boards because of writing to
> unintended locations. So, I'd like to see this discussed and applied
> soon if possible.
> 
> Thanks everyone,
> 
>    Wolfram
> 
> 
>  drivers/memory/renesas-rpc-if.c | 113 ++++++++++++++++++++++----------
>  include/memory/renesas-rpc-if.h |   1 +
>  2 files changed, 79 insertions(+), 35 deletions(-)

You sent the patch just slightly after this one:
https://lore.kernel.org/lkml/20210922184830.29147-1-andrew_gabbasov@mentor.com/

Are you solving similar problem?


Best regards,
Krzysztof

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Hi Krzysztof,

> > So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
> > accessed by 16-bit width.
> > Similar to SMWDR1, SMDDR0/1 registers.
> > In current code, SMWDR0 register is accessed by regmap_write()
> > that only set up to do 32-bit width.
> 
> Is this part something worth splitting to its own patch?

I don't think so because it is related. The patch ensures that a) only
8, 4, 2, or 1 byte blocks are used and b) whatever is used, the access
width to the data registers is proper. Only the combination of both
fixes the data corruption. Also, for backporting, it would be good to
not introduce dependencies, I think.

> You sent the patch just slightly after this one:
> https://lore.kernel.org/lkml/20210922184830.29147-1-andrew_gabbasov@mentor.com/
> 
> Are you solving similar problem?

Unlikely. I do not have HyperFlash on my board, so I can't test. But as
I understand the docs, HyperFlash in deed doubles the granularity from 8
to 16 bits when using memcpy. But I am dealing with MMIO register
accesses in manual mode here.

Thanks for the review,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the patch.

On Wed, Sep 22, 2021 at 10:10 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> This patch fixes 2 problems:
> [1] The output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
>
> This is the sample warning logs when performing mount/umount then
> remount the device with jffs2 format:
> jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> Read 0x00034e00, calculated 0xadb272a7
>
> The reason for issue [1] is that the writing data seems to
> get messed up.
> Data is only completed when the number of bytes is divisible by 4.
> If you only have 3 bytes of data left to write, 1 garbage byte
> is inserted after the end of the write stream.
> If you only have 2 bytes of data left to write, 2 bytes of '00'
> are added into the write stream.
> If you only have 1 byte of data left to write, 2 bytes of '00'
> are added into the write stream. 1 garbage byte is inserted after
> the end of the write stream.
>
> To solve problem [1], data must be written continuously in serial
> and the write stream ends when data is out.
>
> Following HW manual 62.2.15, access to SMWDR0 register should be
> in the same size as the transfer size specified in the SPIDE[3:0]
> bits in the manual mode enable setting register (SMENR).
> Be sure to access from address 0.
>
> So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
> accessed by 16-bit width.
> Similar to SMWDR1, SMDDR0/1 registers.
> In current code, SMWDR0 register is accessed by regmap_write()
> that only set up to do 32-bit width.
>
> To solve problem [2], data must be written 16-bit or 8-bit when
> transferring 1-byte or 2-byte.
>
> Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
> [wsa: refactored to use regmap only via reg_read/reg_write]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Hi,
>
> I could reproduce the issue by a simple:
>
>   $ echo "Hello" > /dev/mtd10
>
> The original BSP patch fixed the issue but mixed regmap-acces with
> ioread/iowrite accesses. So, I refactored it to use custom regmap
> accessors. This keeps the code more readable IMO. With this patch, my
> custom test cases work as well as the JFFS2 remount mentioned in the
> commit message. Tested on a Renesas Condor board (R-Car V3M) and a
> Falcon board (R-Car V3U). I send this as RFC because this is my first
> patch for the RPC code and hope for feedback. The BSP team has been
> contacted as well for comments and testing. Nonetheless, this addresses
> a serious issue which has caused broken boards because of writing to
> unintended locations. So, I'd like to see this discussed and applied
> soon if possible.
>
I hit the exact same issue on RZ/G2L where erase/write operation
screwed some random sectors and made the board un-bootable. With the
patch applied, read/write/erase worked as expected. Below are the logs
on RZ/G2L SMARC EVK.

root@smarc-rzg2l:~# sh -x ./flash.sh
+ cat /proc/mtd
dev:    size   erasesize  name
mtd0: 02000000 00001000 "boot"
mtd1: 02000000 00001000 "user"
+ flashcp -v sample.bin /dev/mtd1
Erasing blocks: 1024/1024 (100%)
Writing data: 4096k/4096k (100%)
Verifying data: 4096k/4096k (100%)
+ dd if=/dev/urandom of=/tmp/sample.bin bs=1024 count=4096
4096+0 records in
4096+0 records out
4194304 bytes (4.2 MB) copied, 0.0786743 s, 53.3 MB/s
+ flash_erase -j -q /dev/mtd1 0 0
+ mount -t jffs2 /dev/mtdblock1 /mnt
+ cp /tmp/sample.bin /mnt
+ ls -ltr /mnt
total 4096
-rw-r--r-- 1 root root 4194304 Sep 20 10:54 sample.bin
+ echo 'test write'
+ umount /mnt
+ mount -t jffs2 /dev/mtdblock1 /mnt
+ ls -ltr /mnt
total 4097
-rw-r--r-- 1 root root      11 Sep 20 10:54 write.txt
-rw-r--r-- 1 root root 4194304 Sep 20 10:54 sample.bin
+ cat /mnt/write.txt
test write
+ umount /mnt

Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
Prabhakar

> Thanks everyone,
>
>    Wolfram
>
>
>  drivers/memory/renesas-rpc-if.c | 113 ++++++++++++++++++++++----------
>  include/memory/renesas-rpc-if.h |   1 +
>  2 files changed, 79 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
> index 45eed659b0c6..77a011d5ff8c 100644
> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c
> @@ -160,10 +160,62 @@ static const struct regmap_access_table rpcif_volatile_table = {
>         .n_yes_ranges   = ARRAY_SIZE(rpcif_volatile_ranges),
>  };
>
> +
> +/*
> + * Custom accessor functions to ensure SMRDR0 and SMWDR0 are always accessed
> + * with proper width. Requires SMENR_SPIDE to be correctly set before!
> + */
> +static int rpcif_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +       struct rpcif *rpc = context;
> +
> +       if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
> +               u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
> +
> +               if (spide == 0x8) {
> +                       *val = readb(rpc->base + reg);
> +                       return 0;
> +               } else if (spide == 0xC) {
> +                       *val = readw(rpc->base + reg);
> +                       return 0;
> +               } else if (spide != 0xF) {
> +                       return -EILSEQ;
> +               }
> +       }
> +
> +       *val = readl(rpc->base + reg);
> +       return 0;
> +
> +}
> +
> +static int rpcif_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +       struct rpcif *rpc = context;
> +
> +       if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
> +               u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
> +
> +               if (spide == 0x8) {
> +                       writeb(val, rpc->base + reg);
> +                       return 0;
> +               } else if (spide == 0xC) {
> +                       writew(val, rpc->base + reg);
> +                       return 0;
> +               } else if (spide != 0xF) {
> +                       return -EILSEQ;
> +               }
> +       }
> +
> +       writel(val, rpc->base + reg);
> +       return 0;
> +}
> +
>  static const struct regmap_config rpcif_regmap_config = {
>         .reg_bits       = 32,
>         .val_bits       = 32,
>         .reg_stride     = 4,
> +       .reg_read       = rpcif_reg_read,
> +       .reg_write      = rpcif_reg_write,
>         .fast_io        = true,
>         .max_register   = RPCIF_PHYINT,
>         .volatile_table = &rpcif_volatile_table,
> @@ -173,17 +225,15 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
>         struct resource *res;
> -       void __iomem *base;
>
>         rpc->dev = dev;
>
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> -       base = devm_ioremap_resource(&pdev->dev, res);
> -       if (IS_ERR(base))
> -               return PTR_ERR(base);
> +       rpc->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(rpc->base))
> +               return PTR_ERR(rpc->base);
>
> -       rpc->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> -                                           &rpcif_regmap_config);
> +       rpc->regmap = devm_regmap_init(&pdev->dev, NULL, rpc, &rpcif_regmap_config);
>         if (IS_ERR(rpc->regmap)) {
>                 dev_err(&pdev->dev,
>                         "failed to init regmap for rpcif, error %ld\n",
> @@ -354,20 +404,16 @@ void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
>                         nbytes = op->data.nbytes;
>                 rpc->xferlen = nbytes;
>
> -               rpc->enable |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes)) |
> -                       RPCIF_SMENR_SPIDB(rpcif_bit_size(op->data.buswidth));
> +               rpc->enable |= RPCIF_SMENR_SPIDB(rpcif_bit_size(op->data.buswidth));
>         }
>  }
>  EXPORT_SYMBOL(rpcif_prepare);
>
>  int rpcif_manual_xfer(struct rpcif *rpc)
>  {
> -       u32 smenr, smcr, pos = 0, max = 4;
> +       u32 smenr, smcr, pos = 0, max = rpc->bus_size == 2 ? 8 : 4;
>         int ret = 0;
>
> -       if (rpc->bus_size == 2)
> -               max = 8;
> -
>         pm_runtime_get_sync(rpc->dev);
>
>         regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> @@ -378,37 +424,36 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>         regmap_write(rpc->regmap, RPCIF_SMOPR, rpc->option);
>         regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
>         regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
> +       regmap_write(rpc->regmap, RPCIF_SMADR, rpc->smadr);
>         smenr = rpc->enable;
>
>         switch (rpc->dir) {
>         case RPCIF_DATA_OUT:
>                 while (pos < rpc->xferlen) {
> -                       u32 nbytes = rpc->xferlen - pos;
> -                       u32 data[2];
> +                       u32 bytes_left = rpc->xferlen - pos;
> +                       u32 nbytes, data[2];
>
>                         smcr = rpc->smcr | RPCIF_SMCR_SPIE;
> -                       if (nbytes > max) {
> -                               nbytes = max;
> +
> +                       /* nbytes may only be 1, 2, 4, or 8 */
> +                       nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
> +                       if (bytes_left > nbytes)
>                                 smcr |= RPCIF_SMCR_SSLKP;
> -                       }
> +
> +                       smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
> +                       regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>
>                         memcpy(data, rpc->buffer + pos, nbytes);
> -                       if (nbytes > 4) {
> +                       if (nbytes == 8) {
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR1,
>                                              data[0]);
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR0,
>                                              data[1]);
> -                       } else if (nbytes > 2) {
> +                       } else {
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR0,
>                                              data[0]);
> -                       } else  {
> -                               regmap_write(rpc->regmap, RPCIF_SMWDR0,
> -                                            data[0] << 16);
>                         }
>
> -                       regmap_write(rpc->regmap, RPCIF_SMADR,
> -                                    rpc->smadr + pos);
> -                       regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>                         regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
>                         ret = wait_msg_xfer_end(rpc);
>                         if (ret)
> @@ -448,14 +493,16 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>                         break;
>                 }
>                 while (pos < rpc->xferlen) {
> -                       u32 nbytes = rpc->xferlen - pos;
> -                       u32 data[2];
> +                       u32 bytes_left = rpc->xferlen - pos;
> +                       u32 nbytes, data[2];
>
> -                       if (nbytes > max)
> -                               nbytes = max;
> +                       /* nbytes may only be 1, 2, 4, or 8 */
> +                       nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
>
>                         regmap_write(rpc->regmap, RPCIF_SMADR,
>                                      rpc->smadr + pos);
> +                       smenr &= ~RPCIF_SMENR_SPIDE(0xF);
> +                       smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
>                         regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>                         regmap_write(rpc->regmap, RPCIF_SMCR,
>                                      rpc->smcr | RPCIF_SMCR_SPIE);
> @@ -463,18 +510,14 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>                         if (ret)
>                                 goto err_out;
>
> -                       if (nbytes > 4) {
> +                       if (nbytes == 8) {
>                                 regmap_read(rpc->regmap, RPCIF_SMRDR1,
>                                             &data[0]);
>                                 regmap_read(rpc->regmap, RPCIF_SMRDR0,
>                                             &data[1]);
> -                       } else if (nbytes > 2) {
> -                               regmap_read(rpc->regmap, RPCIF_SMRDR0,
> -                                           &data[0]);
> -                       } else  {
> +                       } else {
>                                 regmap_read(rpc->regmap, RPCIF_SMRDR0,
>                                             &data[0]);
> -                               data[0] >>= 16;
>                         }
>                         memcpy(rpc->buffer + pos, data, nbytes);
>
> diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
> index e3e770f76f34..77c694a19149 100644
> --- a/include/memory/renesas-rpc-if.h
> +++ b/include/memory/renesas-rpc-if.h
> @@ -59,6 +59,7 @@ struct rpcif_op {
>
>  struct rpcif {
>         struct device *dev;
> +       void __iomem *base;
>         void __iomem *dirmap;
>         struct regmap *regmap;
>         struct reset_control *rstc;
> --
> 2.30.2
>

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Sep 27, 2021 at 10:52 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Sep 22, 2021 at 10:10 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > This patch fixes 2 problems:
> > [1] The output warning logs and data loss when performing
> > mount/umount then remount the device with jffs2 format.
> > [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> >
> > This is the sample warning logs when performing mount/umount then
> > remount the device with jffs2 format:
> > jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> > Read 0x00034e00, calculated 0xadb272a7
> >
> > The reason for issue [1] is that the writing data seems to
> > get messed up.
> > Data is only completed when the number of bytes is divisible by 4.
> > If you only have 3 bytes of data left to write, 1 garbage byte
> > is inserted after the end of the write stream.
> > If you only have 2 bytes of data left to write, 2 bytes of '00'
> > are added into the write stream.
> > If you only have 1 byte of data left to write, 2 bytes of '00'
> > are added into the write stream. 1 garbage byte is inserted after
> > the end of the write stream.
> >
> > To solve problem [1], data must be written continuously in serial
> > and the write stream ends when data is out.
> >
> > Following HW manual 62.2.15, access to SMWDR0 register should be
> > in the same size as the transfer size specified in the SPIDE[3:0]
> > bits in the manual mode enable setting register (SMENR).
> > Be sure to access from address 0.
> >
> > So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
> > accessed by 16-bit width.
> > Similar to SMWDR1, SMDDR0/1 registers.
> > In current code, SMWDR0 register is accessed by regmap_write()
> > that only set up to do 32-bit width.
> >
> > To solve problem [2], data must be written 16-bit or 8-bit when
> > transferring 1-byte or 2-byte.
> >
> > Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
> > [wsa: refactored to use regmap only via reg_read/reg_write]
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >
> > Hi,
> >
> > I could reproduce the issue by a simple:
> >
> >   $ echo "Hello" > /dev/mtd10
> >
> > The original BSP patch fixed the issue but mixed regmap-acces with
> > ioread/iowrite accesses. So, I refactored it to use custom regmap
> > accessors. This keeps the code more readable IMO. With this patch, my
> > custom test cases work as well as the JFFS2 remount mentioned in the
> > commit message. Tested on a Renesas Condor board (R-Car V3M) and a
> > Falcon board (R-Car V3U). I send this as RFC because this is my first
> > patch for the RPC code and hope for feedback. The BSP team has been
> > contacted as well for comments and testing. Nonetheless, this addresses
> > a serious issue which has caused broken boards because of writing to
> > unintended locations. So, I'd like to see this discussed and applied
> > soon if possible.
> >
> I hit the exact same issue on RZ/G2L where erase/write operation
> screwed some random sectors and made the board un-bootable. With the
> patch applied, read/write/erase worked as expected. Below are the logs
> on RZ/G2L SMARC EVK.
>
> root@smarc-rzg2l:~# sh -x ./flash.sh
> + cat /proc/mtd
> dev:    size   erasesize  name
> mtd0: 02000000 00001000 "boot"
> mtd1: 02000000 00001000 "user"
> + flashcp -v sample.bin /dev/mtd1
> Erasing blocks: 1024/1024 (100%)
> Writing data: 4096k/4096k (100%)
> Verifying data: 4096k/4096k (100%)
> + dd if=/dev/urandom of=/tmp/sample.bin bs=1024 count=4096
> 4096+0 records in
> 4096+0 records out
> 4194304 bytes (4.2 MB) copied, 0.0786743 s, 53.3 MB/s
> + flash_erase -j -q /dev/mtd1 0 0
> + mount -t jffs2 /dev/mtdblock1 /mnt
> + cp /tmp/sample.bin /mnt
> + ls -ltr /mnt
> total 4096
> -rw-r--r-- 1 root root 4194304 Sep 20 10:54 sample.bin
> + echo 'test write'
> + umount /mnt
> + mount -t jffs2 /dev/mtdblock1 /mnt
> + ls -ltr /mnt
> total 4097
> -rw-r--r-- 1 root root      11 Sep 20 10:54 write.txt
> -rw-r--r-- 1 root root 4194304 Sep 20 10:54 sample.bin
> + cat /mnt/write.txt
> test write
> + umount /mnt
>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Might be a good idea to update the erase test to make a copy first,
and verify that only the wanted blocks have been affected by the erase.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Lad, Prabhakar]

Hi Geert,

On Mon, Sep 27, 2021 at 10:46 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Sep 27, 2021 at 10:52 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Sep 22, 2021 at 10:10 AM Wolfram Sang
> > <wsa+renesas@sang-engineering.com> wrote:
> > > This patch fixes 2 problems:
> > > [1] The output warning logs and data loss when performing
> > > mount/umount then remount the device with jffs2 format.
> > > [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> > >
> > > This is the sample warning logs when performing mount/umount then
> > > remount the device with jffs2 format:
> > > jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> > > Read 0x00034e00, calculated 0xadb272a7
> > >
> > > The reason for issue [1] is that the writing data seems to
> > > get messed up.
> > > Data is only completed when the number of bytes is divisible by 4.
> > > If you only have 3 bytes of data left to write, 1 garbage byte
> > > is inserted after the end of the write stream.
> > > If you only have 2 bytes of data left to write, 2 bytes of '00'
> > > are added into the write stream.
> > > If you only have 1 byte of data left to write, 2 bytes of '00'
> > > are added into the write stream. 1 garbage byte is inserted after
> > > the end of the write stream.
> > >
> > > To solve problem [1], data must be written continuously in serial
> > > and the write stream ends when data is out.
> > >
> > > Following HW manual 62.2.15, access to SMWDR0 register should be
> > > in the same size as the transfer size specified in the SPIDE[3:0]
> > > bits in the manual mode enable setting register (SMENR).
> > > Be sure to access from address 0.
> > >
> > > So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
> > > accessed by 16-bit width.
> > > Similar to SMWDR1, SMDDR0/1 registers.
> > > In current code, SMWDR0 register is accessed by regmap_write()
> > > that only set up to do 32-bit width.
> > >
> > > To solve problem [2], data must be written 16-bit or 8-bit when
> > > transferring 1-byte or 2-byte.
> > >
> > > Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
> > > [wsa: refactored to use regmap only via reg_read/reg_write]
> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > ---
> > >
> > > Hi,
> > >
> > > I could reproduce the issue by a simple:
> > >
> > >   $ echo "Hello" > /dev/mtd10
> > >
> > > The original BSP patch fixed the issue but mixed regmap-acces with
> > > ioread/iowrite accesses. So, I refactored it to use custom regmap
> > > accessors. This keeps the code more readable IMO. With this patch, my
> > > custom test cases work as well as the JFFS2 remount mentioned in the
> > > commit message. Tested on a Renesas Condor board (R-Car V3M) and a
> > > Falcon board (R-Car V3U). I send this as RFC because this is my first
> > > patch for the RPC code and hope for feedback. The BSP team has been
> > > contacted as well for comments and testing. Nonetheless, this addresses
> > > a serious issue which has caused broken boards because of writing to
> > > unintended locations. So, I'd like to see this discussed and applied
> > > soon if possible.
> > >
> > I hit the exact same issue on RZ/G2L where erase/write operation
> > screwed some random sectors and made the board un-bootable. With the
> > patch applied, read/write/erase worked as expected. Below are the logs
> > on RZ/G2L SMARC EVK.
> >
> > root@smarc-rzg2l:~# sh -x ./flash.sh
> > + cat /proc/mtd
> > dev:    size   erasesize  name
> > mtd0: 02000000 00001000 "boot"
> > mtd1: 02000000 00001000 "user"
> > + flashcp -v sample.bin /dev/mtd1
> > Erasing blocks: 1024/1024 (100%)
> > Writing data: 4096k/4096k (100%)
> > Verifying data: 4096k/4096k (100%)
> > + dd if=/dev/urandom of=/tmp/sample.bin bs=1024 count=4096
> > 4096+0 records in
> > 4096+0 records out
> > 4194304 bytes (4.2 MB) copied, 0.0786743 s, 53.3 MB/s
> > + flash_erase -j -q /dev/mtd1 0 0
> > + mount -t jffs2 /dev/mtdblock1 /mnt
> > + cp /tmp/sample.bin /mnt
> > + ls -ltr /mnt
> > total 4096
> > -rw-r--r-- 1 root root 4194304 Sep 20 10:54 sample.bin
> > + echo 'test write'
> > + umount /mnt
> > + mount -t jffs2 /dev/mtdblock1 /mnt
> > + ls -ltr /mnt
> > total 4097
> > -rw-r--r-- 1 root root      11 Sep 20 10:54 write.txt
> > -rw-r--r-- 1 root root 4194304 Sep 20 10:54 sample.bin
> > + cat /mnt/write.txt
> > test write
> > + umount /mnt
> >
> > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Might be a good idea to update the erase test to make a copy first,
> and verify that only the wanted blocks have been affected by the erase.
>
Right, I have updated my erase test. Below are logs for my erase test
(from a give offset),

root@smarc-rzg2l:~# flashcp -v sample.bin /dev/mtd1
Erasing blocks: 1024/1024 (100%)
Writing data: 4096k/4096k (100%)
Verifying data: 4096k/4096k (100%)
root@smarc-rzg2l:~# flash_erase /dev/mtd1 4096 1
Erasing 4 Kibyte @ 1000 -- 100 % complete
root@smarc-rzg2l:~# hexdump -C /dev/mtd1 | less
00000fd0  c0 2b 03 33 96 ff 87 5b  f7 96 b5 a9 de 57 eb 2f  |.+.3...[.....W./|
00000fe0  11 70 11 f1 71 53 48 94  67 c8 0e 53 34 76 f4 f6  |.p..qSH.g..S4v..|
00000ff0  a0 ec ed 8d 62 f3 f2 5a  d0 0a 66 74 95 a7 91 7b  |....b..Z..ft...{|
00001000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
*
00002000  ae d0 f4 f5 4b 66 b7 6f  2c 09 04 f1 10 58 40 b0  |....Kf.o,....X@.|
00002010  e5 27 6a d7 1f 8e af 8f  ff 57 34 75 6d b7 7d 99  |.'j......W4um.}.|
00002020  d8 e1 88 20 1a 83 37 e3  54 df 72 76 0a ec 1e 76  |... ..7.T.rv...v|
00002030  84 5c 05 f2 88 61 72 cd  34 4a 71 62 68 b7 b0 f6  |.\...ar.4Jqbh...|
00002040  2e eb a5 d6 79 d5 4d 1a  44 26 e9 77 0d 72 fb f3  |....y.M.D&.w.r..|
00002050  36 64 5a a0 44 1a 35 14  79 69 94 78 78 34 f2 04  |6dZ.D.5.yi.xx4..|
00002060  13 91 3a 5c 07 28 61 c8  a7 82 bc f6 7f 87 d4 da  |..:\.(a.........|
00002070  b4 ec 27 b6 f2 7c 07 c8  b3 d3 8b 8e 1f 5e 75 97  |..'..|.......^u.|
00002080  14 e7 ac b0 bd 3a 20 ce  ed 6a be 53 21 a3 7e 64  |.....: ..j.S!.~d|
00002090  99 0b 61 f0 dd 4c f6 90  c0 aa f4 52 8c 67 05 d0  |..a..L.....R.g..|
000020a0  b8 eb 0e 1e b8 40 09 52  ac 23 57 7f bd 94 3b 7a  |.....@.R.#W...;z|
000020b0  8e 8b 10 7a db bc 9f f8  15 dd 41 ac 92 cc b6 3f  |...z......A....?|
000020c0  67 57 dd d0 fc f1 6e 1e  27 d8 4f 62 98 71 74 ea  |gW....n.'.Ob.qt.|
000020d0  8c 62 82 50 8d ed 5b f1  a6 f1 99 7c e9 f1 8e 08  |.b.P..[....|....|
000020e0  48 c7 2d 73 83 03 96 78  f4 64 57 94 95 64 23 c2  |H.-s...x.dW..d#.|
000020f0  6f 53 32 e7 43 1b 5e 25  a8 b0 34 17 1f 33 4d f4  |oS2.C.^%..4..3M.|
00002100  30 95 91 4d f1 06 37 09  71 f3 ce 5d be f8 62 96  |0..M..7.q..]..b.|
00002110  0f d4 26 cb eb 50 a3 4c  81 6f 1c 8a e6 a2 c6 d3  |..&..P.L.o......|

Cheers,
Prabhakar

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

Hi Prabhakar,

> I hit the exact same issue on RZ/G2L where erase/write operation
> screwed some random sectors and made the board un-bootable. With the
> patch applied, read/write/erase worked as expected. Below are the logs
> on RZ/G2L SMARC EVK.

Thanks for testing. Glad it works for you as well!

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Krzysztof Kozlowski]

On 22/09/2021 11:10, Wolfram Sang wrote:
> This patch fixes 2 problems:
> [1] The output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> 
> This is the sample warning logs when performing mount/umount then
> remount the device with jffs2 format:
> jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> Read 0x00034e00, calculated 0xadb272a7
> 
> The reason for issue [1] is that the writing data seems to
> get messed up.
> Data is only completed when the number of bytes is divisible by 4.
> If you only have 3 bytes of data left to write, 1 garbage byte
> is inserted after the end of the write stream.
> If you only have 2 bytes of data left to write, 2 bytes of '00'
> are added into the write stream.
> If you only have 1 byte of data left to write, 2 bytes of '00'
> are added into the write stream. 1 garbage byte is inserted after
> the end of the write stream.
> 
> To solve problem [1], data must be written continuously in serial
> and the write stream ends when data is out.
> 
> Following HW manual 62.2.15, access to SMWDR0 register should be
> in the same size as the transfer size specified in the SPIDE[3:0]
> bits in the manual mode enable setting register (SMENR).
> Be sure to access from address 0.
> 
> So, in 16-bit transfer (SPIDE[3:0]=b'1100), SMWDR0 should be
> accessed by 16-bit width.
> Similar to SMWDR1, SMDDR0/1 registers.
> In current code, SMWDR0 register is accessed by regmap_write()
> that only set up to do 32-bit width.
> 
> To solve problem [2], data must be written 16-bit or 8-bit when
> transferring 1-byte or 2-byte.
> 
> Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
> [wsa: refactored to use regmap only via reg_read/reg_write]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Hi,
> 
> I could reproduce the issue by a simple:
> 
>   $ echo "Hello" > /dev/mtd10
> 
> The original BSP patch fixed the issue but mixed regmap-acces with
> ioread/iowrite accesses. So, I refactored it to use custom regmap
> accessors. This keeps the code more readable IMO. With this patch, my
> custom test cases work as well as the JFFS2 remount mentioned in the
> commit message. Tested on a Renesas Condor board (R-Car V3M) and a
> Falcon board (R-Car V3U). I send this as RFC because this is my first
> patch for the RPC code and hope for feedback. The BSP team has been
> contacted as well for comments and testing. Nonetheless, this addresses
> a serious issue which has caused broken boards because of writing to
> unintended locations. So, I'd like to see this discussed and applied
> soon if possible.
> 
> Thanks everyone,
> 
>    Wolfram
> 

Thanks for testing and comments. Any Fixes tag for it?

Best regards,
Krzysztof

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 134 bytes --]


> Thanks for testing and comments. Any Fixes tag for it?

Sure:

Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver")

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Krzysztof Kozlowski]

On Wed, 22 Sep 2021 11:10:06 +0200, Wolfram Sang wrote:
> This patch fixes 2 problems:
> [1] The output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> 
> This is the sample warning logs when performing mount/umount then
> remount the device with jffs2 format:
> jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> Read 0x00034e00, calculated 0xadb272a7
> 
> [...]

Applied, thanks!

[1/1] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode
      commit: fff53a551db50f5edecaa0b29a64056ab8d2bbca

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

Hi Krzysztof and Wolfram,

CC Sergey, Prabhakar, Andrew,

On Tue, Sep 28, 2021 at 12:36 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
> On Wed, 22 Sep 2021 11:10:06 +0200, Wolfram Sang wrote:
> > This patch fixes 2 problems:
> > [1] The output warning logs and data loss when performing
> > mount/umount then remount the device with jffs2 format.
> > [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> >
> > This is the sample warning logs when performing mount/umount then
> > remount the device with jffs2 format:
> > jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> > Read 0x00034e00, calculated 0xadb272a7
> >
> > [...]
>
> Applied, thanks!
>
> [1/1] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode
>       commit: fff53a551db50f5edecaa0b29a64056ab8d2bbca

While trying to enable support for RPC on Salvator-X(S)[*], I
discovered HyperFLASH detection is broken:

    rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed

Reverting all commits to the RPC driver since Sergey's original
commit 5de15b610f785f0e ("mtd: hyperbus: add Renesas RPC-IF driver")
fixed probing:

    rpc-if-hyperflash: Found 1 x16 devices at 0x0 in 16-bit bank.
Manufacturer ID 0x000001 Chip ID 0x007000
    Amd/Fujitsu Extended Query Table at 0x0040
      Amd/Fujitsu Extended Query version 1.5.
    rpc-if-hyperflash: CFI contains unrecognised boot bank location
(0). Assuming bottom.
    number of CFI chips: 1
    10 fixed-partitions partitions found on MTD device rpc-if-hyperflash
    Creating 10 MTD partitions on "rpc-if-hyperflash":
    0x000000000000-0x000000040000 : "bootparam"
    [...]

However, there's still something wrong, as all HyperFLASH contents read
back as zeros, while the FLASH contains the full boot loader stack.
Bisecting the reverts pointed to this patch, and just reverting this
patch (small whitespace conflict) fixes probing, too. Still, all zeros.

Summary: needs more investigation ;-)

Wolfram: which platform did you use for QSPI testing, so I don't
break that again?

[*] Firmware compiled with RCAR_RPC_HYPERFLASH_LOCKED=0 of course.
    Without that (e.g. old H3 R-Car ES1.0), it crashes with an
    Asynchronous SError Interrupt during driver initialization.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Krzysztof Kozlowski]

On 07/03/2022 17:44, Geert Uytterhoeven wrote:
> Hi Krzysztof and Wolfram,
> 
> CC Sergey, Prabhakar, Andrew,
> 
> On Tue, Sep 28, 2021 at 12:36 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>> On Wed, 22 Sep 2021 11:10:06 +0200, Wolfram Sang wrote:
>>> This patch fixes 2 problems:
>>> [1] The output warning logs and data loss when performing
>>> mount/umount then remount the device with jffs2 format.
>>> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
>>>
>>> This is the sample warning logs when performing mount/umount then
>>> remount the device with jffs2 format:
>>> jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
>>> Read 0x00034e00, calculated 0xadb272a7
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/1] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode
>>       commit: fff53a551db50f5edecaa0b29a64056ab8d2bbca
> 
> While trying to enable support for RPC on Salvator-X(S)[*], I
> discovered HyperFLASH detection is broken:
> 
>     rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
> 
> Reverting all commits to the RPC driver since Sergey's original
> commit 5de15b610f785f0e ("mtd: hyperbus: add Renesas RPC-IF driver")
> fixed probing:
> 
>     rpc-if-hyperflash: Found 1 x16 devices at 0x0 in 16-bit bank.
> Manufacturer ID 0x000001 Chip ID 0x007000
>     Amd/Fujitsu Extended Query Table at 0x0040
>       Amd/Fujitsu Extended Query version 1.5.
>     rpc-if-hyperflash: CFI contains unrecognised boot bank location
> (0). Assuming bottom.
>     number of CFI chips: 1
>     10 fixed-partitions partitions found on MTD device rpc-if-hyperflash
>     Creating 10 MTD partitions on "rpc-if-hyperflash":
>     0x000000000000-0x000000040000 : "bootparam"
>     [...]
> 
> However, there's still something wrong, as all HyperFLASH contents read
> back as zeros, while the FLASH contains the full boot loader stack.
> Bisecting the reverts pointed to this patch, and just reverting this
> patch (small whitespace conflict) fixes probing, too. Still, all zeros.
> 
> Summary: needs more investigation ;-)
> 
> Wolfram: which platform did you use for QSPI testing, so I don't
> break that again?
> 
> [*] Firmware compiled with RCAR_RPC_HYPERFLASH_LOCKED=0 of course.
>     Without that (e.g. old H3 R-Car ES1.0), it crashes with an
>     Asynchronous SError Interrupt during driver initialization.

Thanks for letting me know. This patch is already in mainline, so the
solution is either to revert it or fix it. I will wait for it from you
(as in plural "you" :) ).

Best regards,
Krzysztof

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

Hi Krzysztof,

On Mon, Mar 7, 2022 at 6:47 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
> On 07/03/2022 17:44, Geert Uytterhoeven wrote:
> > On Tue, Sep 28, 2021 at 12:36 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >> On Wed, 22 Sep 2021 11:10:06 +0200, Wolfram Sang wrote:
> >>> This patch fixes 2 problems:
> >>> [1] The output warning logs and data loss when performing
> >>> mount/umount then remount the device with jffs2 format.
> >>> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> >>>
> >>> This is the sample warning logs when performing mount/umount then
> >>> remount the device with jffs2 format:
> >>> jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> >>> Read 0x00034e00, calculated 0xadb272a7
> >>>
> >>> [...]
> >>
> >> Applied, thanks!
> >>
> >> [1/1] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode
> >>       commit: fff53a551db50f5edecaa0b29a64056ab8d2bbca
> >
> > While trying to enable support for RPC on Salvator-X(S)[*], I
> > discovered HyperFLASH detection is broken:
> >
> >     rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed

[...]

> Thanks for letting me know. This patch is already in mainline, so the
> solution is either to revert it or fix it. I will wait for it from you
> (as in plural "you" :) ).

Thanks, that's exactly what I had expected ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]


> Wolfram: which platform did you use for QSPI testing, so I don't
> break that again?

I did my refactoring using an Eagle board and once this worked, I
enabled QSPI on Falcon. All remotely. Condor was another candidate but
it was broken in the lab at that time.

>     Without that (e.g. old H3 R-Car ES1.0), it crashes with an

Frankly, I wouldn't trust ES1.0 as a reliable source for QSPI. Could you
start with the newest Gen3 board you have and then go to previous ones?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Mar 8, 2022 at 9:21 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > Wolfram: which platform did you use for QSPI testing, so I don't
> > break that again?
>
> I did my refactoring using an Eagle board and once this worked, I
> enabled QSPI on Falcon. All remotely. Condor was another candidate but
> it was broken in the lab at that time.

OK, thanks!

> >     Without that (e.g. old H3 R-Car ES1.0), it crashes with an
>
> Frankly, I wouldn't trust ES1.0 as a reliable source for QSPI. Could you
> start with the newest Gen3 board you have and then go to previous ones?

This is not QSPI, but HF.

Building a new firmware for R-Car H3 ES1.0 with HF unlocked will be
complicated, as it is not supported by upstream TF-A.

Note that HF also fails to probe on R-Car M3-W and M3-N ES1.0.
Haven't tried it on R-Car E3 yet.  All those have a (not so new) TF-A,
but built with RCAR_RPC_HYPERFLASH_LOCKED=0.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]


> This is not QSPI, but HF.

Ah, okay.

> Building a new firmware for R-Car H3 ES1.0 with HF unlocked will be
> complicated, as it is not supported by upstream TF-A.

You mean QSPI here?

> Note that HF also fails to probe on R-Car M3-W and M3-N ES1.0.

Do you have this patch form Andrew in your tree:

[PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash

Even if so, I don't think that reverting patches is the solution. As you
could see from Andrew's patch, HyperFlash was also broken before and it
just may need more fixes for Gen3 perhaps? IIRC my patches didn't break
Andrew's tests but maybe we should ask him again. Maybe Andrew has also
some more ideas, I only did QSPI.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Mar 8, 2022 at 9:46 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > This is not QSPI, but HF.
>
> Ah, okay.
>
> > Building a new firmware for R-Car H3 ES1.0 with HF unlocked will be
> > complicated, as it is not supported by upstream TF-A.
>
> You mean QSPI here?

No, HF. Salvator-X(S) boots from HF.

> > Note that HF also fails to probe on R-Car M3-W and M3-N ES1.0.
>
> Do you have this patch form Andrew in your tree:
>
> [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash

Sure I have it. It's in v5.16 ;-)

> Even if so, I don't think that reverting patches is the solution. As you

Sure, a plain revert is definitely not the right solution.

> could see from Andrew's patch, HyperFlash was also broken before and it
> just may need more fixes for Gen3 perhaps? IIRC my patches didn't break
> Andrew's tests but maybe we should ask him again. Maybe Andrew has also
> some more ideas, I only did QSPI.

My understanding from reading the threads is that Andrew's patch and
yours arrived in parallel, and are believed to fix two different and
non-intersecting things (QSPI vs. HF).  I also found no explicit mention
that Andrew tried your patch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

On Mon, Mar 7, 2022 at 5:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Sep 28, 2021 at 12:36 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
> > On Wed, 22 Sep 2021 11:10:06 +0200, Wolfram Sang wrote:
> > > This patch fixes 2 problems:
> > > [1] The output warning logs and data loss when performing
> > > mount/umount then remount the device with jffs2 format.
> > > [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.
> > >
> > > This is the sample warning logs when performing mount/umount then
> > > remount the device with jffs2 format:
> > > jffs2: jffs2_scan_inode_node(): CRC failed on node at 0x031c51d4:
> > > Read 0x00034e00, calculated 0xadb272a7
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [1/1] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode
> >       commit: fff53a551db50f5edecaa0b29a64056ab8d2bbca
>
> While trying to enable support for RPC on Salvator-X(S)[*], I
> discovered HyperFLASH detection is broken:
>
>     rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed

I've sent an RFC fix, see
https://lore.kernel.org/all/27107f2d578b198078df841ee2e4d7b71b183898.1647017136.git.geert+renesas@glider.be

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Sep 22, 2021 at 11:10 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This patch fixes 2 problems:
> [1] The output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.

Revisiting commit fff53a551db50f5e ("memory: renesas-rpc-if: Correct
QSPI data transfer in Manual mode") in  v5.16-rc1...

> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c

>  int rpcif_manual_xfer(struct rpcif *rpc)
>  {
> -       u32 smenr, smcr, pos = 0, max = 4;
> +       u32 smenr, smcr, pos = 0, max = rpc->bus_size == 2 ? 8 : 4;
>         int ret = 0;
>
> -       if (rpc->bus_size == 2)
> -               max = 8;
> -
>         pm_runtime_get_sync(rpc->dev);
>
>         regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> @@ -378,37 +424,36 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>         regmap_write(rpc->regmap, RPCIF_SMOPR, rpc->option);
>         regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
>         regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
> +       regmap_write(rpc->regmap, RPCIF_SMADR, rpc->smadr);
>         smenr = rpc->enable;
>
>         switch (rpc->dir) {
>         case RPCIF_DATA_OUT:
>                 while (pos < rpc->xferlen) {
> -                       u32 nbytes = rpc->xferlen - pos;
> -                       u32 data[2];
> +                       u32 bytes_left = rpc->xferlen - pos;
> +                       u32 nbytes, data[2];
>
>                         smcr = rpc->smcr | RPCIF_SMCR_SPIE;
> -                       if (nbytes > max) {
> -                               nbytes = max;
> +
> +                       /* nbytes may only be 1, 2, 4, or 8 */
> +                       nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
> +                       if (bytes_left > nbytes)
>                                 smcr |= RPCIF_SMCR_SSLKP;
> -                       }
> +
> +                       smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
> +                       regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>
>                         memcpy(data, rpc->buffer + pos, nbytes);
> -                       if (nbytes > 4) {
> +                       if (nbytes == 8) {
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR1,
>                                              data[0]);
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR0,
>                                              data[1]);
> -                       } else if (nbytes > 2) {
> +                       } else {
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR0,
>                                              data[0]);
> -                       } else  {
> -                               regmap_write(rpc->regmap, RPCIF_SMWDR0,
> -                                            data[0] << 16);
>                         }
>
> -                       regmap_write(rpc->regmap, RPCIF_SMADR,
> -                                    rpc->smadr + pos);

Removing this implies SMADR is auto-incrementing for writes...

> -                       regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>                         regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
>                         ret = wait_msg_xfer_end(rpc);
>                         if (ret)
> @@ -448,14 +493,16 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>                         break;
>                 }
>                 while (pos < rpc->xferlen) {
> -                       u32 nbytes = rpc->xferlen - pos;
> -                       u32 data[2];
> +                       u32 bytes_left = rpc->xferlen - pos;
> +                       u32 nbytes, data[2];
>
> -                       if (nbytes > max)
> -                               nbytes = max;
> +                       /* nbytes may only be 1, 2, 4, or 8 */
> +                       nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
>
>                         regmap_write(rpc->regmap, RPCIF_SMADR,
>                                      rpc->smadr + pos);

... while keeping this assumes SMADR is not auto-incrementing for
reads?

Figure 62.17 "Example of Data Transfer Setting Flow in Manual Operating
Mode" does show writing SMADR in each loop iteration.
I cannot find anything about auto-incrementing in the documentation,
except for Figure 62.28 "Write Buffer Usage Sequence", which does
not apply as Linux does not support the write buffer yet.

Given you tested this, and the BSP commit 0d37f69cacb33435 ("memory:
renesas-rpc-if: Correct QSPI data transfer in Manual mode") does the
same, I assume it's working fine?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds