Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07.02.2020 21:17, Sergei Shtylyov wrote:
On 02/07/2020 10:31 PM, Dirk Behme wrote:

[...]
Add the HyperFLash driver for the Renesas RPC-IF.  It's the "front end"
driver using the "back end" APIs in the main driver to talk to the real
hardware.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
[...]
Index: linux/drivers/mtd/hyperbus/rpc-if.c
===================================================================
--- /dev/null
+++ linux/drivers/mtd/hyperbus/rpc-if.c
@@ -0,0 +1,162 @@
[...]
+static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr)
+{
+    struct rpcif_hyperbus *hyperbus =
+        container_of(hbdev, struct rpcif_hyperbus, hbdev);
+    struct rpcif_op op = rpcif_op_tmpl;
+    map_word data;
+
+    op.cmd.opcode = 0xC0;
+    op.addr.val = addr >> 1;
+    op.dummy.buswidth = 1;
+    op.dummy.ncycles = 15;
+    op.data.dir = RPCIF_DATA_IN;
+    op.data.nbytes = 2;
+    op.data.buf.in = &data;
+    rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
+    rpcif_io_xfer(&hyperbus->rpc);
+
+    return be16_to_cpu(data.x[0]);
+}
+
+static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
+                 u16 data)
+{
+    struct rpcif_hyperbus *hyperbus =
+        container_of(hbdev, struct rpcif_hyperbus, hbdev);
+    struct rpcif_op op = rpcif_op_tmpl;
+
+    op.cmd.opcode = 0x40;
+    op.addr.val = addr >> 1;
+    op.data.dir = RPCIF_DATA_OUT;
+    op.data.nbytes = 2;
+    op.data.buf.out = &data;
+    cpu_to_be16s(&data);



Testing this, I found that writing data to the Hyperflash results in swapped _data_ in Hyperflash due to this cpu_to_be16s() conversion:

02 01 04 03 06 05 08 07 ...

Breaking the usage of the data written for other users, i.e. the boot loaders.

On the other hand, dropping this cpu_to_be16s() (and be16_to_cpu() in the read16 above) makes the probing to fail completely.

The topic seems to be that rpcif_hb_write16() handles command _and_ data, and the commands seem to need the conversion.

     The HyperBus spec says the register space is always big-endian but the
                                                                           ^^^ then

again

HypoerFlash doesn't have the register space...

As mentioned, the first idea, dropping the conversion and adding some debug output in the driver [1] results in failed probe [2]. Successful probing of the unmodified driver  results in [3], then.

Seems I need some advice: Why is this conversion for successful probe required?
Why is the first 'QRY' returned by the device not detected by cfi_qry_mode_on()?

     "QRY" is in the MSBs?


Well, even if we have swapping enabled and with this it's in the LSBs, it's not detected in the first run. See the first 5 traces in [3] below.


Is the any possibility to drop the conversion _and_ make the driver probe
successful? Or do we need to split the path the commands and the data are
routed? If so, how?

     I've found some interesting options under the CFI advanced config options,
e.g. "Flash cmd/query data swapping" having MTD_CFI_BE_BYTE_SWAP value in this
item. With this variant chosen, I don't need any byte swapping in the driver
any more... and the QRY signature is read correctly on the very 1st try.


Yes, but ;)

I tried MTD_CFI_BE_BYTE_SWAP config option, too. Enabling that and dropping cpu_to_be16s()/be16_to_cpu() in the driver result in a successful probe. And
/dev/mtdx afterwards. That's the good news.

But, the bad news:

Trying a write (dd to /dev/mtdx) hanged and never returned. In contrast to the

    Not for me:

root@192.168.2.11:~# dd if=jffs2.img of=/dev/mtd11
random: crng init done
2666+1 records in
2666+1 records out
1365320 bytes (1.4 MB) copied, 33.0917 seconds, 41.3 kB/s

solution with the cpu_to_be16s()/be16_to_cpu() in the driver, which wrote nicely to the Hyperflash, but swapped.

    Something's wrong at your end...


Yes, fixed that, working now :)

For reference, I did [1].

Best regards

Dirk

[1]

From af977d8e53cca6f2e20fb737b4c8655d83e2d7c4 Mon Sep 17 00:00:00 2001
From: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
Date: Mon, 10 Feb 2020 09:11:40 +0100
Subject: [PATCH] mtd: hyperbus: rpc-if: Use built in endian conversion

Instead of 'manually' doing the endian conversion in the driver,
use the MTD built in one.

FIXME: How to autoselect MTD_CFI_BE_BYTE_SWAP? 'select MTD_CFI_BE_BYTE_SWAP'
       in Kconfig doesn't seem to work?

Signed-off-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
---
 arch/arm64/configs/rcar3_defconfig | 1 +
 drivers/mtd/hyperbus/Kconfig       | 2 ++
 drivers/mtd/hyperbus/rpc-if.c      | 8 ++++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/configs/rcar3_defconfig b/arch/arm64/configs/rcar3_defconfig
index d04d5bd83580..cf5636b333b9 100644
--- a/arch/arm64/configs/rcar3_defconfig
+++ b/arch/arm64/configs/rcar3_defconfig
@@ -172,6 +172,7 @@ CONFIG_DEVTMPFS_MOUNT=y
 CONFIG_DMA_CMA=y
 CONFIG_CONNECTOR=m
 CONFIG_MTD=y
+CONFIG_MTD_CFI_BE_BYTE_SWAP=y
 CONFIG_MTD_PHYSMAP_OF=y
 CONFIG_MTD_M25P80=m
 CONFIG_MTD_SPI_NOR=m
diff --git a/drivers/mtd/hyperbus/Kconfig b/drivers/mtd/hyperbus/Kconfig
index d80489d9989c..353be8c8f339 100644
--- a/drivers/mtd/hyperbus/Kconfig
+++ b/drivers/mtd/hyperbus/Kconfig
@@ -25,6 +25,8 @@ config HBMC_AM654
 config RPCIF_HYPERBUS
 	tristate "Renesas RPC-IF HyperBus driver"
 	depends on RENESAS_RPCIF
+	select MTD_CFI_ADV_OPTIONS
+	select MTD_CFI_BE_BYTE_SWAP
 	help
 	  This option includes Renesas RPC-IF HyperFlash support.

diff --git a/drivers/mtd/hyperbus/rpc-if.c b/drivers/mtd/hyperbus/rpc-if.c
index a66a5080b482..6e0c45b5ef95 100644
--- a/drivers/mtd/hyperbus/rpc-if.c
+++ b/drivers/mtd/hyperbus/rpc-if.c
@@ -17,6 +17,11 @@

 #include <memory/renesas-rpc-if.h>

+/* FIXME: How to drop this? */
+#if  !defined(CONFIG_MTD_CFI_BE_BYTE_SWAP)
+#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)"
+#endif
+
 struct	rpcif_hyperbus {
 	struct rpcif rpc;
 	struct hyperbus_ctlr ctlr;
@@ -60,7 +65,7 @@ static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr)
 	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
 	rpcif_io_xfer(&hyperbus->rpc);

-	return be16_to_cpu(data.x[0]);
+	return data.x[0];
 }

static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr, @@ -75,7 +80,6 @@ static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
 	op.data.dir = RPCIF_DATA_OUT;
 	op.data.nbytes = 2;
 	op.data.buf.out = &data;
-	cpu_to_be16s(&data);
 	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
 	rpcif_io_xfer(&hyperbus->rpc);
 }
--
2.20.0



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/



[Index of Archives]     [LARTC]     [Bugtraq]     [Yosemite Forum]     [Photo]

  Powered by Linux