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

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@cogentembedded.com>

---
 drivers/mtd/hyperbus/Kconfig  |    6 +
 drivers/mtd/hyperbus/Makefile |    1 
 drivers/mtd/hyperbus/rpc-if.c |  162 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)

Hi Sergei,

On 30/01/20 2:09 am, Sergei Shtylyov 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@cogentembedded.com>
> 
> ---

Is the backend merged? Or are the patches still in RFC stage?

Regards
Vignesh

>  drivers/mtd/hyperbus/Kconfig  |    6 +
>  drivers/mtd/hyperbus/Makefile |    1 
>  drivers/mtd/hyperbus/rpc-if.c |  162 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 169 insertions(+)
> 
> Index: linux/drivers/mtd/hyperbus/Kconfig
> ===================================================================
> --- linux.orig/drivers/mtd/hyperbus/Kconfig
> +++ linux/drivers/mtd/hyperbus/Kconfig
> @@ -22,4 +22,10 @@ config HBMC_AM654
>  	 This is the driver for HyperBus controller on TI's AM65x and
>  	 other SoCs
>  
> +config RPCIF_HYPERBUS
> +	tristate "Renesas RPC-IF HyperBus driver"
> +	depends on RENESAS_RPCIF
> +	help
> +	  This option includes Renesas RPC-IF HyperFlash support.
> +
>  endif # MTD_HYPERBUS
> Index: linux/drivers/mtd/hyperbus/Makefile
> ===================================================================
> --- linux.orig/drivers/mtd/hyperbus/Makefile
> +++ linux/drivers/mtd/hyperbus/Makefile
> @@ -2,3 +2,4 @@
>  
>  obj-$(CONFIG_MTD_HYPERBUS)	+= hyperbus-core.o
>  obj-$(CONFIG_HBMC_AM654)	+= hbmc-am654.o
> +obj-$(CONFIG_RPCIF_HYPERBUS)	+= rpc-if.o
> Index: linux/drivers/mtd/hyperbus/rpc-if.c
> ===================================================================
> --- /dev/null
> +++ linux/drivers/mtd/hyperbus/rpc-if.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Linux driver for RPC-IF HyperFlash
> + *
> + * Copyright (C) 2019 Cogent Embedded, Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <memory/renesas-rpc-if.h>
> +
> +struct	rpcif_hyperbus {
> +	struct rpcif rpc;
> +	struct hyperbus_ctlr ctlr;
> +	struct hyperbus_device hbdev;
> +};
> +
> +static const struct rpcif_op rpcif_op_tmpl = {
> +	.cmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.ocmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.addr = {
> +		.nbytes = 1,
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.data = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +};
> +
> +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);
> +	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
> +	rpcif_io_xfer(&hyperbus->rpc);
> +}
> +
> +static void rpcif_hb_copy_from(struct hyperbus_device *hbdev, void *to,
> +			       unsigned long from, ssize_t len)
> +{
> +	struct rpcif_hyperbus *hyperbus =
> +		container_of(hbdev, struct rpcif_hyperbus, hbdev);
> +	struct rpcif_op op = rpcif_op_tmpl;
> +
> +	op.cmd.opcode = 0xA0;
> +	op.addr.val = from;
> +	op.dummy.buswidth = 1;
> +	op.dummy.ncycles = 15;
> +	op.data.dir = RPCIF_DATA_IN;
> +	op.data.nbytes = len;
> +	op.data.buf.in = to;
> +	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
> +	rpcif_dirmap_read(&hyperbus->rpc, from, len, to);
> +}
> +
> +static const struct hyperbus_ops rpcif_hb_ops = {
> +	.read16 = rpcif_hb_read16,
> +	.write16 = rpcif_hb_write16,
> +	.copy_from = rpcif_hb_copy_from,
> +};
> +
> +static int rpcif_hb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rpcif_hyperbus *hyperbus;
> +	int status;
> +
> +	hyperbus = devm_kzalloc(dev, sizeof(*hyperbus), GFP_KERNEL);
> +	if (!hyperbus)
> +		return -ENOMEM;
> +
> +	rpcif_sw_init(&hyperbus->rpc, pdev->dev.parent);
> +
> +	platform_set_drvdata(pdev, hyperbus);
> +
> +	rpcif_enable_rpm(&hyperbus->rpc);
> +
> +	rpcif_hw_init(&hyperbus->rpc, true);
> +
> +	hyperbus->hbdev.map.size = hyperbus->rpc.size;
> +	hyperbus->hbdev.map.virt = hyperbus->rpc.dirmap;
> +
> +	hyperbus->ctlr.dev = dev;
> +	hyperbus->ctlr.ops = &rpcif_hb_ops;
> +	hyperbus->hbdev.ctlr = &hyperbus->ctlr;
> +	hyperbus->hbdev.np = of_get_next_child(pdev->dev.parent->of_node, NULL);
> +	status = hyperbus_register_device(&hyperbus->hbdev);
> +	if (status) {
> +		dev_err(dev, "failed to register device\n");
> +		rpcif_disable_rpm(&hyperbus->rpc);
> +	}
> +
> +	return status;
> +}
> +
> +static int rpcif_hb_remove(struct platform_device *pdev)
> +{
> +	struct rpcif_hyperbus *hyperbus = platform_get_drvdata(pdev);
> +	int error = hyperbus_unregister_device(&hyperbus->hbdev);
> +	struct rpcif *rpc = dev_get_drvdata(pdev->dev.parent);
> +
> +	rpcif_disable_rpm(rpc);
> +	return error;
> +}
> +
> +static struct platform_driver rpcif_platform_driver = {
> +	.probe	= rpcif_hb_probe,
> +	.remove	= rpcif_hb_remove,
> +	.driver	= {
> +		.name	= "rpc-if-hyperflash",
> +	},
> +};
> +
> +module_platform_driver(rpcif_platform_driver);
> +
> +MODULE_DESCRIPTION("Renesas RPC-IF HyperFlash driver");
> +MODULE_LICENSE("GPL v2");
>

On 02/03/2020 07:59 AM, Vignesh Raghavendra wrote:
> Hi Sergei,
> 
> On 30/01/20 2:09 am, Sergei Shtylyov 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@cogentembedded.com>
>>
>> ---
> 
> Is the backend merged? Or are the patches still in RFC stage?

   No, it's still the same RFC patch, and I'm not sure who'd merge it --
drivers/memory/ is unmaintained. Perhaps the best way would be to merge it
along with the SPI front end?

MBR, Sergei

Hi,

On 29.01.2020 21:39, Sergei Shtylyov 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@cogentembedded.com>
> 
> ---
>   drivers/mtd/hyperbus/Kconfig  |    6 +
>   drivers/mtd/hyperbus/Makefile |    1
>   drivers/mtd/hyperbus/rpc-if.c |  162 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 169 insertions(+)
> 
> Index: linux/drivers/mtd/hyperbus/Kconfig
> ===================================================================
> --- linux.orig/drivers/mtd/hyperbus/Kconfig
> +++ linux/drivers/mtd/hyperbus/Kconfig
> @@ -22,4 +22,10 @@ config HBMC_AM654
>   	 This is the driver for HyperBus controller on TI's AM65x and
>   	 other SoCs
>   
> +config RPCIF_HYPERBUS
> +	tristate "Renesas RPC-IF HyperBus driver"
> +	depends on RENESAS_RPCIF
> +	help
> +	  This option includes Renesas RPC-IF HyperFlash support.
> +
>   endif # MTD_HYPERBUS
> Index: linux/drivers/mtd/hyperbus/Makefile
> ===================================================================
> --- linux.orig/drivers/mtd/hyperbus/Makefile
> +++ linux/drivers/mtd/hyperbus/Makefile
> @@ -2,3 +2,4 @@
>   
>   obj-$(CONFIG_MTD_HYPERBUS)	+= hyperbus-core.o
>   obj-$(CONFIG_HBMC_AM654)	+= hbmc-am654.o
> +obj-$(CONFIG_RPCIF_HYPERBUS)	+= rpc-if.o
> Index: linux/drivers/mtd/hyperbus/rpc-if.c
> ===================================================================
> --- /dev/null
> +++ linux/drivers/mtd/hyperbus/rpc-if.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Linux driver for RPC-IF HyperFlash
> + *
> + * Copyright (C) 2019 Cogent Embedded, Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <memory/renesas-rpc-if.h>
> +
> +struct	rpcif_hyperbus {
> +	struct rpcif rpc;
> +	struct hyperbus_ctlr ctlr;
> +	struct hyperbus_device hbdev;
> +};
> +
> +static const struct rpcif_op rpcif_op_tmpl = {
> +	.cmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.ocmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.addr = {
> +		.nbytes = 1,
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.data = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +};
> +
> +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.

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()? 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?

Many questions ;)

Best regards

Dirk


[1] Dropping be16_to_cpu() & cpu_to_be16s() and adding some debug output:

diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
index 6f16552cd59f..e5dd8dd3b594 100644
--- a/drivers/mtd/chips/cfi_util.c
+++ b/drivers/mtd/chips/cfi_util.c
@@ -239,9 +239,13 @@ int __xipram cfi_qry_present(struct map_info *map, 
__u32 base,
  }
  EXPORT_SYMBOL_GPL(cfi_qry_present);

+static unsigned int count = 1;
+
  int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
                              struct cfi_private *cfi)
  {
+       pr_err("cfi_qry_mode_on() called #%i\n", count++);
+
         cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
         cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, 
NULL);
         if (cfi_qry_present(map, base, cfi))
@@ -273,6 +277,9 @@ int __xipram cfi_qry_mode_on(uint32_t base, struct 
map_info *map,
         if (cfi_qry_present(map, base, cfi))
                 return 1;
         /* QRY not found */
+
+       pr_err("cfi_qry_mode_on() failed\n");
+
         return 0;
  }
  EXPORT_SYMBOL_GPL(cfi_qry_mode_on);
diff --git a/drivers/mtd/hyperbus/rpc-if.c b/drivers/mtd/hyperbus/rpc-if.c
index a66a5080b482..bb83a8f3f3bc 100644
--- a/drivers/mtd/hyperbus/rpc-if.c
+++ b/drivers/mtd/hyperbus/rpc-if.c
@@ -60,7 +60,11 @@ 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]);
+       pr_err("read:  a: 0x%08lX  d: 0x%04X %c %c\n", addr, (unsigned 
short)data.x[0],
+              (unsigned char)((data.x[0] >> 8) & 0xFF),
+              (unsigned char)data.x[0]);
+
+       return data.x[0];
  }

  static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned 
long addr,
@@ -75,7 +79,7 @@ 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);
+       pr_err("write: a: 0x%08lX  d: 0x%04X\n", addr, data);
         rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
         rpcif_io_xfer(&hyperbus->rpc);
  }


[2] Probe fails without be16_to_cpu/cpu_to_be16s:

cfi_qry_mode_on() called #1
write: a: 0x00000000  d: 0xF0F0
write: a: 0x000000AA  d: 0x9898
read:  a: 0x00000020  d: 0x5100 Q
read:  a: 0x00000022  d: 0x5200 R
read:  a: 0x00000024  d: 0x5900 Y
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000000  d: 0xFFFF
write: a: 0x000000AA  d: 0x9898
read:  a: 0x00000020  d: 0x5100 Q
read:  a: 0x00000022  d: 0x5200 R
read:  a: 0x00000024  d: 0x5900 Y
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000AAA  d: 0x9898
read:  a: 0x00000020  d: 0x5100 Q
read:  a: 0x00000022  d: 0x5200 R
read:  a: 0x00000024  d: 0x5900 Y
write: a: 0x00000000  d: 0xF0F0
write: a: 0x0000AAAA  d: 0xAAAA
write: a: 0x00005554  d: 0x5555
write: a: 0x0000AAAA  d: 0x9898
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000AAA  d: 0xAAAA
write: a: 0x00000554  d: 0x5555
write: a: 0x00000AAA  d: 0x9898
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #2
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000154  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000000  d: 0xFFFF
write: a: 0x00000154  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00001554  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00015554  d: 0xAAAA
write: a: 0x0000AAAA  d: 0x5555
write: a: 0x00015554  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00001554  d: 0xAAAA
write: a: 0x00000AAA  d: 0x5555
write: a: 0x00001554  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #3
write: a: 0x00000000  d: 0xF0F0
write: a: 0x000002A8  d: 0x9898
read:  a: 0x00000080  d: 0xF358 ¦ X
read:  a: 0x00000088  d: 0x0057  W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000000  d: 0xFFFF
write: a: 0x000002A8  d: 0x9898
read:  a: 0x00000080  d: 0xF358 ¦ X
read:  a: 0x00000088  d: 0x0057  W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00002AA8  d: 0x9898
read:  a: 0x00000080  d: 0xF358 ¦ X
read:  a: 0x00000088  d: 0x0057  W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x0002AAA8  d: 0xAAAA
write: a: 0x00015554  d: 0x5555
write: a: 0x0002AAA8  d: 0x9898
read:  a: 0x00000080  d: 0xF358 ¦ X
read:  a: 0x00000088  d: 0x0057  W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00002AA8  d: 0xAAAA
write: a: 0x00001554  d: 0x5555
write: a: 0x00002AA8  d: 0x9898
read:  a: 0x00000080  d: 0xF358 ¦ X
read:  a: 0x00000088  d: 0x0057  W
read:  a: 0x00000090  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #4
write: a: 0x00000000  d: 0x00F0
write: a: 0x000000AA  d: 0x0098
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00000000  d: 0x00FF
write: a: 0x000000AA  d: 0x0098
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00000AAA  d: 0x0098
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x0000AAAA  d: 0x00AA
write: a: 0x00005554  d: 0x0055
write: a: 0x0000AAAA  d: 0x0098
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00000AAA  d: 0x00AA
write: a: 0x00000554  d: 0x0055
write: a: 0x00000AAA  d: 0x0098
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #5
write: a: 0x00000000  d: 0x00F0
write: a: 0x00000154  d: 0x0098
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00000000  d: 0x00FF
write: a: 0x00000154  d: 0x0098
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00001554  d: 0x0098
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00015554  d: 0x00AA
write: a: 0x0000AAAA  d: 0x0055
write: a: 0x00015554  d: 0x0098
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0x00F0
write: a: 0x00001554  d: 0x00AA
write: a: 0x00000AAA  d: 0x0055
write: a: 0x00001554  d: 0x0098
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
cfi_qry_mode_on() failed
rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
rpc-if-hyperflash rpc-if-hyperflash: failed to register device



[3] Probe success WITH be16_to_cpu/cpu_to_be16s:

cfi_qry_mode_on() called #1
write: a: 0x00000000  d: 0xF0F0
write: a: 0x000000AA  d: 0x9898
read:  a: 0x00000020  d: 0x0051  Q
read:  a: 0x00000022  d: 0x0052  R
read:  a: 0x00000024  d: 0x0059  Y
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000000  d: 0xFFFF
write: a: 0x000000AA  d: 0x9898
read:  a: 0x00000020  d: 0x0051  Q
read:  a: 0x00000022  d: 0x0052  R
read:  a: 0x00000024  d: 0x0059  Y
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000AAA  d: 0x9898
read:  a: 0x00000020  d: 0x0051  Q
read:  a: 0x00000022  d: 0x0052  R
read:  a: 0x00000024  d: 0x0059  Y
write: a: 0x00000000  d: 0xF0F0
write: a: 0x0000AAAA  d: 0xAAAA
write: a: 0x00005554  d: 0x5555
write: a: 0x0000AAAA  d: 0x9898
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000AAA  d: 0xAAAA
write: a: 0x00000554  d: 0x5555
write: a: 0x00000AAA  d: 0x9898
read:  a: 0x00000020  d: 0x0000
read:  a: 0x00000022  d: 0x0000
read:  a: 0x00000024  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #2
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000154  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000000  d: 0xFFFF
write: a: 0x00000154  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00001554  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00015554  d: 0xAAAA
write: a: 0x0000AAAA  d: 0x5555
write: a: 0x00015554  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00001554  d: 0xAAAA
write: a: 0x00000AAA  d: 0x5555
write: a: 0x00001554  d: 0x9898
read:  a: 0x00000040  d: 0x0000
read:  a: 0x00000044  d: 0x0000
read:  a: 0x00000048  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #3
write: a: 0x00000000  d: 0xF0F0
write: a: 0x000002A8  d: 0x9898
read:  a: 0x00000080  d: 0x58F3 X ¦
read:  a: 0x00000088  d: 0x5700 W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00000000  d: 0xFFFF
write: a: 0x000002A8  d: 0x9898
read:  a: 0x00000080  d: 0x58F3 X ¦
read:  a: 0x00000088  d: 0x5700 W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00002AA8  d: 0x9898
read:  a: 0x00000080  d: 0x58F3 X ¦
read:  a: 0x00000088  d: 0x5700 W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x0002AAA8  d: 0xAAAA
write: a: 0x00015554  d: 0x5555
write: a: 0x0002AAA8  d: 0x9898
read:  a: 0x00000080  d: 0x58F3 X ¦
read:  a: 0x00000088  d: 0x5700 W
read:  a: 0x00000090  d: 0x0000
write: a: 0x00000000  d: 0xF0F0
write: a: 0x00002AA8  d: 0xAAAA
write: a: 0x00001554  d: 0x5555
write: a: 0x00002AA8  d: 0x9898
read:  a: 0x00000080  d: 0x58F3 X ¦
read:  a: 0x00000088  d: 0x5700 W
read:  a: 0x00000090  d: 0x0000
cfi_qry_mode_on() failed
cfi_qry_mode_on() called #4
write: a: 0x00000000  d: 0xF000
write: a: 0x000000AA  d: 0x9800
read:  a: 0x00000020  d: 0x0051  Q
read:  a: 0x00000022  d: 0x0052  R
read:  a: 0x00000024  d: 0x0059  Y
read:  a: 0x00000058  d: 0x0001
read:  a: 0x00000020  d: 0x0051  Q
read:  a: 0x00000022  d: 0x0052  R
read:  a: 0x00000024  d: 0x0059  Y
read:  a: 0x00000026  d: 0x0002
read:  a: 0x00000028  d: 0x0000
read:  a: 0x0000002A  d: 0x0040  @
read:  a: 0x0000002C  d: 0x0000
read:  a: 0x0000002E  d: 0x0000
read:  a: 0x00000030  d: 0x0000
read:  a: 0x00000032  d: 0x0000
read:  a: 0x00000034  d: 0x0000
read:  a: 0x00000036  d: 0x0017
read:  a: 0x00000038  d: 0x0019
read:  a: 0x0000003A  d: 0x0000
read:  a: 0x0000003C  d: 0x0000
read:  a: 0x0000003E  d: 0x0009
read:  a: 0x00000040  d: 0x0009
read:  a: 0x00000042  d: 0x000A

read:  a: 0x00000044  d: 0x0012
read:  a: 0x00000046  d: 0x0002
read:  a: 0x00000048  d: 0x0002
read:  a: 0x0000004A  d: 0x0002
read:  a: 0x0000004C  d: 0x0002
read:  a: 0x0000004E  d: 0x001A
read:  a: 0x00000050  d: 0x0000
read:  a: 0x00000052  d: 0x0000
read:  a: 0x00000054  d: 0x0009
read:  a: 0x00000056  d: 0x0000
read:  a: 0x00000058  d: 0x0001
read:  a: 0x0000005A  d: 0x00FF  ¦
read:  a: 0x0000005C  d: 0x0000
read:  a: 0x0000005E  d: 0x0000
read:  a: 0x00000060  d: 0x0004
write: a: 0x00000000  d: 0xF000
write: a: 0x00000AAA  d: 0xAA00
write: a: 0x00000554  d: 0x5500
write: a: 0x00000AAA  d: 0x9000
read:  a: 0x00000000  d: 0x0001
read:  a: 0x00000002  d: 0x007E  ~
read:  a: 0x0000001C  d: 0x0070  p
read:  a: 0x0000001E  d: 0x0000
write: a: 0x00000000  d: 0xF000
write: a: 0x00000000  d: 0xFF00
cfi_qry_mode_on() called #5
write: a: 0x00000000  d: 0xF000
write: a: 0x000000AA  d: 0x9800
read:  a: 0x00000020  d: 0x0051  Q
read:  a: 0x00000022  d: 0x0052  R
read:  a: 0x00000024  d: 0x0059  Y
read:  a: 0x00000080  d: 0x0050  P
read:  a: 0x00000082  d: 0x0052  R
read:  a: 0x00000084  d: 0x0049  I
read:  a: 0x00000086  d: 0x0031  1
read:  a: 0x00000088  d: 0x0035  5
read:  a: 0x0000008A  d: 0x001C
read:  a: 0x0000008C  d: 0x0002
read:  a: 0x0000008E  d: 0x0001
read:  a: 0x00000090  d: 0x0000
read:  a: 0x00000092  d: 0x0008
read:  a: 0x00000094  d: 0x0000
read:  a: 0x00000096  d: 0x0001
read:  a: 0x00000098  d: 0x0000
read:  a: 0x0000009A  d: 0x0000
read:  a: 0x0000009C  d: 0x0000
read:  a: 0x0000009E  d: 0x0000
write: a: 0x00000000  d: 0xF000
write: a: 0x00000000  d: 0xFF00
=> success

Hello!

On 02/07/2020 03:59 PM, Behme Dirk (CM/ESO2) 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@cogentembedded.com>
[...]
>> 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 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?

> 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.

> Many questions ;)

   Hopefully an answer was found.

> Best regards
> 
> Dirk
> 
> 
> [1] Dropping be16_to_cpu() & cpu_to_be16s() and adding some debug output:
> 
> diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
> index 6f16552cd59f..e5dd8dd3b594 100644
> --- a/drivers/mtd/chips/cfi_util.c
> +++ b/drivers/mtd/chips/cfi_util.c
> @@ -239,9 +239,13 @@ int __xipram cfi_qry_present(struct map_info *map, __u32 base,
>  }
>  EXPORT_SYMBOL_GPL(cfi_qry_present);
> 
> +static unsigned int count = 1;
> +
>  int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
>                              struct cfi_private *cfi)
>  {
> +       pr_err("cfi_qry_mode_on() called #%i\n", count++);
> +
>         cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
>         cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
>         if (cfi_qry_present(map, base, cfi))
> @@ -273,6 +277,9 @@ int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
>         if (cfi_qry_present(map, base, cfi))
>                 return 1;
>         /* QRY not found */
> +
> +       pr_err("cfi_qry_mode_on() failed\n");
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(cfi_qry_mode_on);
> diff --git a/drivers/mtd/hyperbus/rpc-if.c b/drivers/mtd/hyperbus/rpc-if.c
> index a66a5080b482..bb83a8f3f3bc 100644
> --- a/drivers/mtd/hyperbus/rpc-if.c
> +++ b/drivers/mtd/hyperbus/rpc-if.c
> @@ -60,7 +60,11 @@ 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]);
> +       pr_err("read:  a: 0x%08lX  d: 0x%04X %c %c\n", addr, (unsigned short)data.x[0],
> +              (unsigned char)((data.x[0] >> 8) & 0xFF),
> +              (unsigned char)data.x[0]);
> +
> +       return data.x[0];
>  }
> 
>  static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
> @@ -75,7 +79,7 @@ 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);
> +       pr_err("write: a: 0x%08lX  d: 0x%04X\n", addr, data);
>         rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>         rpcif_io_xfer(&hyperbus->rpc);
>  }
> 
> 
> [2] Probe fails without be16_to_cpu/cpu_to_be16s:
> 
> cfi_qry_mode_on() called #1
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x5100 Q
> read:  a: 0x00000022  d: 0x5200 R
> read:  a: 0x00000024  d: 0x5900 Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x5100 Q
> read:  a: 0x00000022  d: 0x5200 R
> read:  a: 0x00000024  d: 0x5900 Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x5100 Q
> read:  a: 0x00000022  d: 0x5200 R
> read:  a: 0x00000024  d: 0x5900 Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0000AAAA  d: 0xAAAA
> write: a: 0x00005554  d: 0x5555
> write: a: 0x0000AAAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0xAAAA
> write: a: 0x00000554  d: 0x5555
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #2
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00015554  d: 0xAAAA
> write: a: 0x0000AAAA  d: 0x5555
> write: a: 0x00015554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0xAAAA
> write: a: 0x00000AAA  d: 0x5555
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #3
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0002AAA8  d: 0xAAAA
> write: a: 0x00015554  d: 0x5555
> write: a: 0x0002AAA8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0xAAAA
> write: a: 0x00001554  d: 0x5555
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #4
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x000000AA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000000  d: 0x00FF
> write: a: 0x000000AA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000AAA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x0000AAAA  d: 0x00AA
> write: a: 0x00005554  d: 0x0055
> write: a: 0x0000AAAA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000AAA  d: 0x00AA
> write: a: 0x00000554  d: 0x0055
> write: a: 0x00000AAA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #5
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000154  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000000  d: 0x00FF
> write: a: 0x00000154  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00001554  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00015554  d: 0x00AA
> write: a: 0x0000AAAA  d: 0x0055
> write: a: 0x00015554  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00001554  d: 0x00AA
> write: a: 0x00000AAA  d: 0x0055
> write: a: 0x00001554  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> cfi_qry_mode_on() failed
> rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
> rpc-if-hyperflash rpc-if-hyperflash: failed to register device
> 
> 
> 
> [3] Probe success WITH be16_to_cpu/cpu_to_be16s:
> 
> cfi_qry_mode_on() called #1
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0000AAAA  d: 0xAAAA
> write: a: 0x00005554  d: 0x5555
> write: a: 0x0000AAAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0xAAAA
> write: a: 0x00000554  d: 0x5555
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #2
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00015554  d: 0xAAAA
> write: a: 0x0000AAAA  d: 0x5555
> write: a: 0x00015554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0xAAAA
> write: a: 0x00000AAA  d: 0x5555
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #3
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0002AAA8  d: 0xAAAA
> write: a: 0x00015554  d: 0x5555
> write: a: 0x0002AAA8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0xAAAA
> write: a: 0x00001554  d: 0x5555
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #4
> write: a: 0x00000000  d: 0xF000
> write: a: 0x000000AA  d: 0x9800
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> read:  a: 0x00000058  d: 0x0001
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> read:  a: 0x00000026  d: 0x0002
> read:  a: 0x00000028  d: 0x0000
> read:  a: 0x0000002A  d: 0x0040  @
> read:  a: 0x0000002C  d: 0x0000
> read:  a: 0x0000002E  d: 0x0000
> read:  a: 0x00000030  d: 0x0000
> read:  a: 0x00000032  d: 0x0000
> read:  a: 0x00000034  d: 0x0000
> read:  a: 0x00000036  d: 0x0017
> read:  a: 0x00000038  d: 0x0019
> read:  a: 0x0000003A  d: 0x0000
> read:  a: 0x0000003C  d: 0x0000
> read:  a: 0x0000003E  d: 0x0009
> read:  a: 0x00000040  d: 0x0009
> read:  a: 0x00000042  d: 0x000A
> 
> read:  a: 0x00000044  d: 0x0012
> read:  a: 0x00000046  d: 0x0002
> read:  a: 0x00000048  d: 0x0002
> read:  a: 0x0000004A  d: 0x0002
> read:  a: 0x0000004C  d: 0x0002
> read:  a: 0x0000004E  d: 0x001A
> read:  a: 0x00000050  d: 0x0000
> read:  a: 0x00000052  d: 0x0000
> read:  a: 0x00000054  d: 0x0009
> read:  a: 0x00000056  d: 0x0000
> read:  a: 0x00000058  d: 0x0001
> read:  a: 0x0000005A  d: 0x00FF  ¦
> read:  a: 0x0000005C  d: 0x0000
> read:  a: 0x0000005E  d: 0x0000
> read:  a: 0x00000060  d: 0x0004
> write: a: 0x00000000  d: 0xF000
> write: a: 0x00000AAA  d: 0xAA00
> write: a: 0x00000554  d: 0x5500
> write: a: 0x00000AAA  d: 0x9000
> read:  a: 0x00000000  d: 0x0001
> read:  a: 0x00000002  d: 0x007E  ~
> read:  a: 0x0000001C  d: 0x0070  p
> read:  a: 0x0000001E  d: 0x0000
> write: a: 0x00000000  d: 0xF000
> write: a: 0x00000000  d: 0xFF00
> cfi_qry_mode_on() called #5
> write: a: 0x00000000  d: 0xF000
> write: a: 0x000000AA  d: 0x9800
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> read:  a: 0x00000080  d: 0x0050  P
> read:  a: 0x00000082  d: 0x0052  R
> read:  a: 0x00000084  d: 0x0049  I
> read:  a: 0x00000086  d: 0x0031  1
> read:  a: 0x00000088  d: 0x0035  5
> read:  a: 0x0000008A  d: 0x001C
> read:  a: 0x0000008C  d: 0x0002
> read:  a: 0x0000008E  d: 0x0001
> read:  a: 0x00000090  d: 0x0000
> read:  a: 0x00000092  d: 0x0008
> read:  a: 0x00000094  d: 0x0000
> read:  a: 0x00000096  d: 0x0001
> read:  a: 0x00000098  d: 0x0000
> read:  a: 0x0000009A  d: 0x0000
> read:  a: 0x0000009C  d: 0x0000
> read:  a: 0x0000009E  d: 0x0000
> write: a: 0x00000000  d: 0xF000
> write: a: 0x00000000  d: 0xFF00
> => success
> 
>

Hi Sergei,

On 07.02.20 20:09, Sergei Shtylyov wrote:
> Hello!
> 
> On 02/07/2020 03:59 PM, Behme Dirk (CM/ESO2) 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@cogentembedded.com>
> [...]
>>> 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 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 solution with the cpu_to_be16s()/be16_to_cpu() in the 
driver, which wrote nicely to the Hyperflash, but swapped.

Best regards

Dirk


>> Many questions ;)
> 
>     Hopefully an answer was found.
> 
>> Best regards
>>
>> Dirk
>>
>>
>> [1] Dropping be16_to_cpu() & cpu_to_be16s() and adding some debug output:
>>
>> diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
>> index 6f16552cd59f..e5dd8dd3b594 100644
>> --- a/drivers/mtd/chips/cfi_util.c
>> +++ b/drivers/mtd/chips/cfi_util.c
>> @@ -239,9 +239,13 @@ int __xipram cfi_qry_present(struct map_info *map, __u32 base,
>>   }
>>   EXPORT_SYMBOL_GPL(cfi_qry_present);
>>
>> +static unsigned int count = 1;
>> +
>>   int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
>>                               struct cfi_private *cfi)
>>   {
>> +       pr_err("cfi_qry_mode_on() called #%i\n", count++);
>> +
>>          cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
>>          cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
>>          if (cfi_qry_present(map, base, cfi))
>> @@ -273,6 +277,9 @@ int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
>>          if (cfi_qry_present(map, base, cfi))
>>                  return 1;
>>          /* QRY not found */
>> +
>> +       pr_err("cfi_qry_mode_on() failed\n");
>> +
>>          return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(cfi_qry_mode_on);
>> diff --git a/drivers/mtd/hyperbus/rpc-if.c b/drivers/mtd/hyperbus/rpc-if.c
>> index a66a5080b482..bb83a8f3f3bc 100644
>> --- a/drivers/mtd/hyperbus/rpc-if.c
>> +++ b/drivers/mtd/hyperbus/rpc-if.c
>> @@ -60,7 +60,11 @@ 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]);
>> +       pr_err("read:  a: 0x%08lX  d: 0x%04X %c %c\n", addr, (unsigned short)data.x[0],
>> +              (unsigned char)((data.x[0] >> 8) & 0xFF),
>> +              (unsigned char)data.x[0]);
>> +
>> +       return data.x[0];
>>   }
>>
>>   static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
>> @@ -75,7 +79,7 @@ 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);
>> +       pr_err("write: a: 0x%08lX  d: 0x%04X\n", addr, data);
>>          rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>>          rpcif_io_xfer(&hyperbus->rpc);
>>   }
>>
>>
>> [2] Probe fails without be16_to_cpu/cpu_to_be16s:
>>
>> cfi_qry_mode_on() called #1
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x000000AA  d: 0x9898
>> read:  a: 0x00000020  d: 0x5100 Q
>> read:  a: 0x00000022  d: 0x5200 R
>> read:  a: 0x00000024  d: 0x5900 Y
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000000  d: 0xFFFF
>> write: a: 0x000000AA  d: 0x9898
>> read:  a: 0x00000020  d: 0x5100 Q
>> read:  a: 0x00000022  d: 0x5200 R
>> read:  a: 0x00000024  d: 0x5900 Y
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000AAA  d: 0x9898
>> read:  a: 0x00000020  d: 0x5100 Q
>> read:  a: 0x00000022  d: 0x5200 R
>> read:  a: 0x00000024  d: 0x5900 Y
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x0000AAAA  d: 0xAAAA
>> write: a: 0x00005554  d: 0x5555
>> write: a: 0x0000AAAA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000AAA  d: 0xAAAA
>> write: a: 0x00000554  d: 0x5555
>> write: a: 0x00000AAA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #2
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000154  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000000  d: 0xFFFF
>> write: a: 0x00000154  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00001554  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00015554  d: 0xAAAA
>> write: a: 0x0000AAAA  d: 0x5555
>> write: a: 0x00015554  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00001554  d: 0xAAAA
>> write: a: 0x00000AAA  d: 0x5555
>> write: a: 0x00001554  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #3
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x000002A8  d: 0x9898
>> read:  a: 0x00000080  d: 0xF358 ¦ X
>> read:  a: 0x00000088  d: 0x0057  W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000000  d: 0xFFFF
>> write: a: 0x000002A8  d: 0x9898
>> read:  a: 0x00000080  d: 0xF358 ¦ X
>> read:  a: 0x00000088  d: 0x0057  W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00002AA8  d: 0x9898
>> read:  a: 0x00000080  d: 0xF358 ¦ X
>> read:  a: 0x00000088  d: 0x0057  W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x0002AAA8  d: 0xAAAA
>> write: a: 0x00015554  d: 0x5555
>> write: a: 0x0002AAA8  d: 0x9898
>> read:  a: 0x00000080  d: 0xF358 ¦ X
>> read:  a: 0x00000088  d: 0x0057  W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00002AA8  d: 0xAAAA
>> write: a: 0x00001554  d: 0x5555
>> write: a: 0x00002AA8  d: 0x9898
>> read:  a: 0x00000080  d: 0xF358 ¦ X
>> read:  a: 0x00000088  d: 0x0057  W
>> read:  a: 0x00000090  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #4
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x000000AA  d: 0x0098
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00000000  d: 0x00FF
>> write: a: 0x000000AA  d: 0x0098
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00000AAA  d: 0x0098
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x0000AAAA  d: 0x00AA
>> write: a: 0x00005554  d: 0x0055
>> write: a: 0x0000AAAA  d: 0x0098
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00000AAA  d: 0x00AA
>> write: a: 0x00000554  d: 0x0055
>> write: a: 0x00000AAA  d: 0x0098
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #5
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00000154  d: 0x0098
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00000000  d: 0x00FF
>> write: a: 0x00000154  d: 0x0098
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00001554  d: 0x0098
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00015554  d: 0x00AA
>> write: a: 0x0000AAAA  d: 0x0055
>> write: a: 0x00015554  d: 0x0098
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0x00F0
>> write: a: 0x00001554  d: 0x00AA
>> write: a: 0x00000AAA  d: 0x0055
>> write: a: 0x00001554  d: 0x0098
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> cfi_qry_mode_on() failed
>> rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
>> rpc-if-hyperflash rpc-if-hyperflash: failed to register device
>>
>>
>>
>> [3] Probe success WITH be16_to_cpu/cpu_to_be16s:
>>
>> cfi_qry_mode_on() called #1
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x000000AA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0051  Q
>> read:  a: 0x00000022  d: 0x0052  R
>> read:  a: 0x00000024  d: 0x0059  Y
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000000  d: 0xFFFF
>> write: a: 0x000000AA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0051  Q
>> read:  a: 0x00000022  d: 0x0052  R
>> read:  a: 0x00000024  d: 0x0059  Y
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000AAA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0051  Q
>> read:  a: 0x00000022  d: 0x0052  R
>> read:  a: 0x00000024  d: 0x0059  Y
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x0000AAAA  d: 0xAAAA
>> write: a: 0x00005554  d: 0x5555
>> write: a: 0x0000AAAA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000AAA  d: 0xAAAA
>> write: a: 0x00000554  d: 0x5555
>> write: a: 0x00000AAA  d: 0x9898
>> read:  a: 0x00000020  d: 0x0000
>> read:  a: 0x00000022  d: 0x0000
>> read:  a: 0x00000024  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #2
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000154  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000000  d: 0xFFFF
>> write: a: 0x00000154  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00001554  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00015554  d: 0xAAAA
>> write: a: 0x0000AAAA  d: 0x5555
>> write: a: 0x00015554  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00001554  d: 0xAAAA
>> write: a: 0x00000AAA  d: 0x5555
>> write: a: 0x00001554  d: 0x9898
>> read:  a: 0x00000040  d: 0x0000
>> read:  a: 0x00000044  d: 0x0000
>> read:  a: 0x00000048  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #3
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x000002A8  d: 0x9898
>> read:  a: 0x00000080  d: 0x58F3 X ¦
>> read:  a: 0x00000088  d: 0x5700 W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00000000  d: 0xFFFF
>> write: a: 0x000002A8  d: 0x9898
>> read:  a: 0x00000080  d: 0x58F3 X ¦
>> read:  a: 0x00000088  d: 0x5700 W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00002AA8  d: 0x9898
>> read:  a: 0x00000080  d: 0x58F3 X ¦
>> read:  a: 0x00000088  d: 0x5700 W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x0002AAA8  d: 0xAAAA
>> write: a: 0x00015554  d: 0x5555
>> write: a: 0x0002AAA8  d: 0x9898
>> read:  a: 0x00000080  d: 0x58F3 X ¦
>> read:  a: 0x00000088  d: 0x5700 W
>> read:  a: 0x00000090  d: 0x0000
>> write: a: 0x00000000  d: 0xF0F0
>> write: a: 0x00002AA8  d: 0xAAAA
>> write: a: 0x00001554  d: 0x5555
>> write: a: 0x00002AA8  d: 0x9898
>> read:  a: 0x00000080  d: 0x58F3 X ¦
>> read:  a: 0x00000088  d: 0x5700 W
>> read:  a: 0x00000090  d: 0x0000
>> cfi_qry_mode_on() failed
>> cfi_qry_mode_on() called #4
>> write: a: 0x00000000  d: 0xF000
>> write: a: 0x000000AA  d: 0x9800
>> read:  a: 0x00000020  d: 0x0051  Q
>> read:  a: 0x00000022  d: 0x0052  R
>> read:  a: 0x00000024  d: 0x0059  Y
>> read:  a: 0x00000058  d: 0x0001
>> read:  a: 0x00000020  d: 0x0051  Q
>> read:  a: 0x00000022  d: 0x0052  R
>> read:  a: 0x00000024  d: 0x0059  Y
>> read:  a: 0x00000026  d: 0x0002
>> read:  a: 0x00000028  d: 0x0000
>> read:  a: 0x0000002A  d: 0x0040  @
>> read:  a: 0x0000002C  d: 0x0000
>> read:  a: 0x0000002E  d: 0x0000
>> read:  a: 0x00000030  d: 0x0000
>> read:  a: 0x00000032  d: 0x0000
>> read:  a: 0x00000034  d: 0x0000
>> read:  a: 0x00000036  d: 0x0017
>> read:  a: 0x00000038  d: 0x0019
>> read:  a: 0x0000003A  d: 0x0000
>> read:  a: 0x0000003C  d: 0x0000
>> read:  a: 0x0000003E  d: 0x0009
>> read:  a: 0x00000040  d: 0x0009
>> read:  a: 0x00000042  d: 0x000A
>>
>> read:  a: 0x00000044  d: 0x0012
>> read:  a: 0x00000046  d: 0x0002
>> read:  a: 0x00000048  d: 0x0002
>> read:  a: 0x0000004A  d: 0x0002
>> read:  a: 0x0000004C  d: 0x0002
>> read:  a: 0x0000004E  d: 0x001A
>> read:  a: 0x00000050  d: 0x0000
>> read:  a: 0x00000052  d: 0x0000
>> read:  a: 0x00000054  d: 0x0009
>> read:  a: 0x00000056  d: 0x0000
>> read:  a: 0x00000058  d: 0x0001
>> read:  a: 0x0000005A  d: 0x00FF  ¦
>> read:  a: 0x0000005C  d: 0x0000
>> read:  a: 0x0000005E  d: 0x0000
>> read:  a: 0x00000060  d: 0x0004
>> write: a: 0x00000000  d: 0xF000
>> write: a: 0x00000AAA  d: 0xAA00
>> write: a: 0x00000554  d: 0x5500
>> write: a: 0x00000AAA  d: 0x9000
>> read:  a: 0x00000000  d: 0x0001
>> read:  a: 0x00000002  d: 0x007E  ~
>> read:  a: 0x0000001C  d: 0x0070  p
>> read:  a: 0x0000001E  d: 0x0000
>> write: a: 0x00000000  d: 0xF000
>> write: a: 0x00000000  d: 0xFF00
>> cfi_qry_mode_on() called #5
>> write: a: 0x00000000  d: 0xF000
>> write: a: 0x000000AA  d: 0x9800
>> read:  a: 0x00000020  d: 0x0051  Q
>> read:  a: 0x00000022  d: 0x0052  R
>> read:  a: 0x00000024  d: 0x0059  Y
>> read:  a: 0x00000080  d: 0x0050  P
>> read:  a: 0x00000082  d: 0x0052  R
>> read:  a: 0x00000084  d: 0x0049  I
>> read:  a: 0x00000086  d: 0x0031  1
>> read:  a: 0x00000088  d: 0x0035  5
>> read:  a: 0x0000008A  d: 0x001C
>> read:  a: 0x0000008C  d: 0x0002
>> read:  a: 0x0000008E  d: 0x0001
>> read:  a: 0x00000090  d: 0x0000
>> read:  a: 0x00000092  d: 0x0008
>> read:  a: 0x00000094  d: 0x0000
>> read:  a: 0x00000096  d: 0x0001
>> read:  a: 0x00000098  d: 0x0000
>> read:  a: 0x0000009A  d: 0x0000
>> read:  a: 0x0000009C  d: 0x0000
>> read:  a: 0x0000009E  d: 0x0000
>> write: a: 0x00000000  d: 0xF000
>> write: a: 0x00000000  d: 0xFF00
>> => success
>>
>>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

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@cogentembedded.com>
>> [...]
>>>> 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...

> Best regards
> 
> Dirk

MBR, Sergei

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@cogentembedded.com>
>>> [...]
>>>>> 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@de.bosch.com>
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@de.bosch.com>
---
  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);
  }

Hi Sergei

On 30/01/20 2:09 am, Sergei Shtylyov 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@cogentembedded.com>
> 
> ---
>  drivers/mtd/hyperbus/Kconfig  |    6 +
>  drivers/mtd/hyperbus/Makefile |    1 
>  drivers/mtd/hyperbus/rpc-if.c |  162 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 169 insertions(+)
> 
> Index: linux/drivers/mtd/hyperbus/Kconfig
> ===================================================================
> --- linux.orig/drivers/mtd/hyperbus/Kconfig
> +++ linux/drivers/mtd/hyperbus/Kconfig
> @@ -22,4 +22,10 @@ config HBMC_AM654
>  	 This is the driver for HyperBus controller on TI's AM65x and
>  	 other SoCs
>  
> +config RPCIF_HYPERBUS
> +	tristate "Renesas RPC-IF HyperBus driver"
> +	depends on RENESAS_RPCIF
> +	help
> +	  This option includes Renesas RPC-IF HyperFlash support.
> +
>  endif # MTD_HYPERBUS
> Index: linux/drivers/mtd/hyperbus/Makefile
> ===================================================================
> --- linux.orig/drivers/mtd/hyperbus/Makefile
> +++ linux/drivers/mtd/hyperbus/Makefile
> @@ -2,3 +2,4 @@
>  
>  obj-$(CONFIG_MTD_HYPERBUS)	+= hyperbus-core.o
>  obj-$(CONFIG_HBMC_AM654)	+= hbmc-am654.o
> +obj-$(CONFIG_RPCIF_HYPERBUS)	+= rpc-if.o
> Index: linux/drivers/mtd/hyperbus/rpc-if.c
> ===================================================================
> --- /dev/null
> +++ linux/drivers/mtd/hyperbus/rpc-if.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Linux driver for RPC-IF HyperFlash
> + *
> + * Copyright (C) 2019 Cogent Embedded, Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <memory/renesas-rpc-if.h>
> +
> +struct	rpcif_hyperbus {
> +	struct rpcif rpc;
> +	struct hyperbus_ctlr ctlr;
> +	struct hyperbus_device hbdev;
> +};
> +
> +static const struct rpcif_op rpcif_op_tmpl = {
> +	.cmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.ocmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.addr = {
> +		.nbytes = 1,
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.data = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +};
> +

Looking around, there seems to be more than one SPI controllers, apart
from Renesas, which also support SPI NOR and HyperFlash protocol within
a single IP block. E.g.: Cadence xSPI controller [1]. Therefore, we need
a generic framework to support these kind of controllers.

One way would be to extend spi_mem_op to support above template along
with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
core can then register a spi_device and use spi-mem ops to talk to
controller driver.
So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
driver (instead of driver/memory) and use extended spi_mem_op to support
HyperFlash.


[1]
https://ip.cadence.com/uploads/1244/cdn-dsd-mem-fla-host-controller-ip-for-xspi-pdf


Regards
Vignesh

> +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);
> +	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
> +	rpcif_io_xfer(&hyperbus->rpc);
> +}
> +
> +static void rpcif_hb_copy_from(struct hyperbus_device *hbdev, void *to,
> +			       unsigned long from, ssize_t len)
> +{
> +	struct rpcif_hyperbus *hyperbus =
> +		container_of(hbdev, struct rpcif_hyperbus, hbdev);
> +	struct rpcif_op op = rpcif_op_tmpl;
> +
> +	op.cmd.opcode = 0xA0;
> +	op.addr.val = from;
> +	op.dummy.buswidth = 1;
> +	op.dummy.ncycles = 15;
> +	op.data.dir = RPCIF_DATA_IN;
> +	op.data.nbytes = len;
> +	op.data.buf.in = to;
> +	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
> +	rpcif_dirmap_read(&hyperbus->rpc, from, len, to);
> +}
> +
> +static const struct hyperbus_ops rpcif_hb_ops = {
> +	.read16 = rpcif_hb_read16,
> +	.write16 = rpcif_hb_write16,
> +	.copy_from = rpcif_hb_copy_from,
> +};
> +
> +static int rpcif_hb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rpcif_hyperbus *hyperbus;
> +	int status;
> +
> +	hyperbus = devm_kzalloc(dev, sizeof(*hyperbus), GFP_KERNEL);
> +	if (!hyperbus)
> +		return -ENOMEM;
> +
> +	rpcif_sw_init(&hyperbus->rpc, pdev->dev.parent);
> +
> +	platform_set_drvdata(pdev, hyperbus);
> +
> +	rpcif_enable_rpm(&hyperbus->rpc);
> +
> +	rpcif_hw_init(&hyperbus->rpc, true);
> +
> +	hyperbus->hbdev.map.size = hyperbus->rpc.size;
> +	hyperbus->hbdev.map.virt = hyperbus->rpc.dirmap;
> +
> +	hyperbus->ctlr.dev = dev;
> +	hyperbus->ctlr.ops = &rpcif_hb_ops;
> +	hyperbus->hbdev.ctlr = &hyperbus->ctlr;
> +	hyperbus->hbdev.np = of_get_next_child(pdev->dev.parent->of_node, NULL);
> +	status = hyperbus_register_device(&hyperbus->hbdev);
> +	if (status) {
> +		dev_err(dev, "failed to register device\n");
> +		rpcif_disable_rpm(&hyperbus->rpc);
> +	}
> +
> +	return status;
> +}
> +
> +static int rpcif_hb_remove(struct platform_device *pdev)
> +{
> +	struct rpcif_hyperbus *hyperbus = platform_get_drvdata(pdev);
> +	int error = hyperbus_unregister_device(&hyperbus->hbdev);
> +	struct rpcif *rpc = dev_get_drvdata(pdev->dev.parent);
> +
> +	rpcif_disable_rpm(rpc);
> +	return error;
> +}
> +
> +static struct platform_driver rpcif_platform_driver = {
> +	.probe	= rpcif_hb_probe,
> +	.remove	= rpcif_hb_remove,
> +	.driver	= {
> +		.name	= "rpc-if-hyperflash",
> +	},
> +};
> +
> +module_platform_driver(rpcif_platform_driver);
> +
> +MODULE_DESCRIPTION("Renesas RPC-IF HyperFlash driver");
> +MODULE_LICENSE("GPL v2");
>

Hi Vignesh,

On 18.02.2020 05:00, Vignesh Raghavendra wrote:
> Hi Sergei
> 
> On 30/01/20 2:09 am, Sergei Shtylyov 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@cogentembedded.com>
>>
>> ---
>>   drivers/mtd/hyperbus/Kconfig  |    6 +
>>   drivers/mtd/hyperbus/Makefile |    1
>>   drivers/mtd/hyperbus/rpc-if.c |  162 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 169 insertions(+)
>>
>> Index: linux/drivers/mtd/hyperbus/Kconfig
>> ===================================================================
>> --- linux.orig/drivers/mtd/hyperbus/Kconfig
>> +++ linux/drivers/mtd/hyperbus/Kconfig
>> @@ -22,4 +22,10 @@ config HBMC_AM654
>>   	 This is the driver for HyperBus controller on TI's AM65x and
>>   	 other SoCs
>>   
>> +config RPCIF_HYPERBUS
>> +	tristate "Renesas RPC-IF HyperBus driver"
>> +	depends on RENESAS_RPCIF
>> +	help
>> +	  This option includes Renesas RPC-IF HyperFlash support.
>> +
>>   endif # MTD_HYPERBUS
>> Index: linux/drivers/mtd/hyperbus/Makefile
>> ===================================================================
>> --- linux.orig/drivers/mtd/hyperbus/Makefile
>> +++ linux/drivers/mtd/hyperbus/Makefile
>> @@ -2,3 +2,4 @@
>>   
>>   obj-$(CONFIG_MTD_HYPERBUS)	+= hyperbus-core.o
>>   obj-$(CONFIG_HBMC_AM654)	+= hbmc-am654.o
>> +obj-$(CONFIG_RPCIF_HYPERBUS)	+= rpc-if.o
>> Index: linux/drivers/mtd/hyperbus/rpc-if.c
>> ===================================================================
>> --- /dev/null
>> +++ linux/drivers/mtd/hyperbus/rpc-if.c
>> @@ -0,0 +1,162 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Linux driver for RPC-IF HyperFlash
>> + *
>> + * Copyright (C) 2019 Cogent Embedded, Inc.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/hyperbus.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/types.h>
>> +
>> +#include <memory/renesas-rpc-if.h>
>> +
>> +struct	rpcif_hyperbus {
>> +	struct rpcif rpc;
>> +	struct hyperbus_ctlr ctlr;
>> +	struct hyperbus_device hbdev;
>> +};
>> +
>> +static const struct rpcif_op rpcif_op_tmpl = {
>> +	.cmd = {
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +	.ocmd = {
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +	.addr = {
>> +		.nbytes = 1,
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +	.data = {
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +};
>> +
> 
> Looking around, there seems to be more than one SPI controllers, apart
> from Renesas, which also support SPI NOR and HyperFlash protocol within
> a single IP block. E.g.: Cadence xSPI controller [1]. Therefore, we need
> a generic framework to support these kind of controllers.
> 
> One way would be to extend spi_mem_op to support above template along
> with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
> core can then register a spi_device and use spi-mem ops to talk to
> controller driver.
> So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
> driver (instead of driver/memory) and use extended spi_mem_op to support
> HyperFlash.


 From Renesas Hyperflash user point of view, I wonder if a two step 
approach would be possible and acceptable, here?

Being a user of the Renesas Hyperflash, I want a driver for that. And, 
of course, I want it "now" ;)

So I wonder if it would be a valid option to have a functioning Renesas 
Hypeflash driver, first. And in a second step abstract that in a more 
generic way to support additional controllers. While in parallel having 
a functional driver for the Renesas people, already.

Is the support for [1] a more or less theoretical one, at the moment? Or 
are there users of that which need support "now", too?

Best regards

Dirk


> [1]
> https://ip.cadence.com/uploads/1244/cdn-dsd-mem-fla-host-controller-ip-for-xspi-pdf
> 
> 
> Regards
> Vignesh
> 
>> +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);
>> +	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>> +	rpcif_io_xfer(&hyperbus->rpc);
>> +}
>> +
>> +static void rpcif_hb_copy_from(struct hyperbus_device *hbdev, void *to,
>> +			       unsigned long from, ssize_t len)
>> +{
>> +	struct rpcif_hyperbus *hyperbus =
>> +		container_of(hbdev, struct rpcif_hyperbus, hbdev);
>> +	struct rpcif_op op = rpcif_op_tmpl;
>> +
>> +	op.cmd.opcode = 0xA0;
>> +	op.addr.val = from;
>> +	op.dummy.buswidth = 1;
>> +	op.dummy.ncycles = 15;
>> +	op.data.dir = RPCIF_DATA_IN;
>> +	op.data.nbytes = len;
>> +	op.data.buf.in = to;
>> +	rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>> +	rpcif_dirmap_read(&hyperbus->rpc, from, len, to);
>> +}
>> +
>> +static const struct hyperbus_ops rpcif_hb_ops = {
>> +	.read16 = rpcif_hb_read16,
>> +	.write16 = rpcif_hb_write16,
>> +	.copy_from = rpcif_hb_copy_from,
>> +};
>> +
>> +static int rpcif_hb_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rpcif_hyperbus *hyperbus;
>> +	int status;
>> +
>> +	hyperbus = devm_kzalloc(dev, sizeof(*hyperbus), GFP_KERNEL);
>> +	if (!hyperbus)
>> +		return -ENOMEM;
>> +
>> +	rpcif_sw_init(&hyperbus->rpc, pdev->dev.parent);
>> +
>> +	platform_set_drvdata(pdev, hyperbus);
>> +
>> +	rpcif_enable_rpm(&hyperbus->rpc);
>> +
>> +	rpcif_hw_init(&hyperbus->rpc, true);
>> +
>> +	hyperbus->hbdev.map.size = hyperbus->rpc.size;
>> +	hyperbus->hbdev.map.virt = hyperbus->rpc.dirmap;
>> +
>> +	hyperbus->ctlr.dev = dev;
>> +	hyperbus->ctlr.ops = &rpcif_hb_ops;
>> +	hyperbus->hbdev.ctlr = &hyperbus->ctlr;
>> +	hyperbus->hbdev.np = of_get_next_child(pdev->dev.parent->of_node, NULL);
>> +	status = hyperbus_register_device(&hyperbus->hbdev);
>> +	if (status) {
>> +		dev_err(dev, "failed to register device\n");
>> +		rpcif_disable_rpm(&hyperbus->rpc);
>> +	}
>> +
>> +	return status;
>> +}
>> +
>> +static int rpcif_hb_remove(struct platform_device *pdev)
>> +{
>> +	struct rpcif_hyperbus *hyperbus = platform_get_drvdata(pdev);
>> +	int error = hyperbus_unregister_device(&hyperbus->hbdev);
>> +	struct rpcif *rpc = dev_get_drvdata(pdev->dev.parent);
>> +
>> +	rpcif_disable_rpm(rpc);
>> +	return error;
>> +}
>> +
>> +static struct platform_driver rpcif_platform_driver = {
>> +	.probe	= rpcif_hb_probe,
>> +	.remove	= rpcif_hb_remove,
>> +	.driver	= {
>> +		.name	= "rpc-if-hyperflash",
>> +	},
>> +};
>> +
>> +module_platform_driver(rpcif_platform_driver);
>> +
>> +MODULE_DESCRIPTION("Renesas RPC-IF HyperFlash driver");
>> +MODULE_LICENSE("GPL v2");

Hi,

On 18/02/20 12:42 pm, Behme Dirk (CM/ESO2) wrote:
> Hi Vignesh,
> 
> On 18.02.2020 05:00, Vignesh Raghavendra wrote:
>> Hi Sergei
>>
[...]
>>
>> Looking around, there seems to be more than one SPI controllers, apart
>> from Renesas, which also support SPI NOR and HyperFlash protocol within
>> a single IP block. E.g.: Cadence xSPI controller [1]. Therefore, we need
>> a generic framework to support these kind of controllers.
>>
>> One way would be to extend spi_mem_op to support above template along
>> with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
>> core can then register a spi_device and use spi-mem ops to talk to
>> controller driver.
>> So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
>> driver (instead of driver/memory) and use extended spi_mem_op to support
>> HyperFlash.
> 
> 
> From Renesas Hyperflash user point of view, I wonder if a two step
> approach would be possible and acceptable, here?
> 
> Being a user of the Renesas Hyperflash, I want a driver for that. And,
> of course, I want it "now" ;)
> 
> So I wonder if it would be a valid option to have a functioning Renesas
> Hypeflash driver, first. And in a second step abstract that in a more
> generic way to support additional controllers. While in parallel having
> a functional driver for the Renesas people, already.
> 

AFAICS, the backend driver is not merged and is still in RFC phase.
Therefore I don't see any benefit of two step approach here. Besides
you'll have to throw away this new driver (hyperbus/rpc-if.c) entirely
later on.

How difficult is it to rewrite backend to be spi-mem driver? There is
already has a spi_mem_ops frontend implementation, so I don't see much
of an issue.
Extending hyperbus core to use spi-mem should also straight forward
Would involve moving this patch into core file.

> Is the support for [1] a more or less theoretical one, at the moment? Or
> are there users of that which need support "now", too?
> 

Its not theoretical, I do see patches for xSPI controller here:
https://patchwork.kernel.org/cover/11354193/

So, its best to sort this out now so as to avoid possible backward
compatibility issues (especially with DT bindings)

Regards
Vignesh

Hello!

On 02/18/2020 07:00 AM, Vignesh Raghavendra 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@cogentembedded.com>
[...]
>> Index: linux/drivers/mtd/hyperbus/rpc-if.c
>> ===================================================================
>> --- /dev/null
>> +++ linux/drivers/mtd/hyperbus/rpc-if.c
>> @@ -0,0 +1,162 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Linux driver for RPC-IF HyperFlash
>> + *
>> + * Copyright (C) 2019 Cogent Embedded, Inc.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/hyperbus.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/types.h>
>> +
>> +#include <memory/renesas-rpc-if.h>
>> +
>> +struct	rpcif_hyperbus {
>> +	struct rpcif rpc;
>> +	struct hyperbus_ctlr ctlr;
>> +	struct hyperbus_device hbdev;
>> +};
>> +
>> +static const struct rpcif_op rpcif_op_tmpl = {
>> +	.cmd = {
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +	.ocmd = {
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +	.addr = {
>> +		.nbytes = 1,
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +	.data = {
>> +		.buswidth = 8,
>> +		.ddr = true,
>> +	},
>> +};
>> +
> 
> Looking around, there seems to be more than one SPI controllers, apart
> from Renesas, which also support SPI NOR and HyperFlash protocol within
> a single IP block. E.g.: Cadence xSPI controller [1]. Therefore, we need
> a generic framework to support these kind of controllers.

   We can use e.g. 'struct rpcif_op' as generic command description.

> One way would be to extend spi_mem_op to support above template along
> with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
> core can then register a spi_device and use spi-mem ops to talk to
> controller driver.

   We have discussed this idea with Mark Brown, the SPI maintainer, and
he wasn't terribly impressed (I've invited him to #mtd -- his nick is
broonie and mine is headless, I'm also adding him to CC:).

> So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
> driver (instead of driver/memory) and use extended spi_mem_op to support
> HyperFlash.

   I don't think cramming support for the different flash busses into
the SPI drivers is a good idea... I'm not against generalizing the
drivers/memory/ APIs though.

> [1]
> https://ip.cadence.com/uploads/1244/cdn-dsd-mem-fla-host-controller-ip-for-xspi-pdf

   Do they have the full datasheet available? I'll try looking at the driver 
tomorrow...

> Regards
> Vignesh

[removed the patch you haven't replied to]

MBR, Sergei

Hi,

On 20/02/20 1:43 am, Sergei Shtylyov wrote:
[...]
>>> +static const struct rpcif_op rpcif_op_tmpl = {
>>> +	.cmd = {
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +	.ocmd = {
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +	.addr = {
>>> +		.nbytes = 1,
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +	.data = {
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +};
>>> +
>>
>> Looking around, there seems to be more than one SPI controllers, apart
>> from Renesas, which also support SPI NOR and HyperFlash protocol within
>> a single IP block. E.g.: Cadence xSPI controller [1]. Therefore, we need
>> a generic framework to support these kind of controllers.
> 
>    We can use e.g. 'struct rpcif_op' as generic command description.
> 
>> One way would be to extend spi_mem_op to support above template along
>> with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
>> core can then register a spi_device and use spi-mem ops to talk to
>> controller driver.
> 
>    We have discussed this idea with Mark Brown, the SPI maintainer, and
> he wasn't terribly impressed (I've invited him to #mtd -- his nick is
> broonie and mine is headless, I'm also adding him to CC:).
> 

I don't see HyperFlash to be very different than Octal DDR SPI NOR
flashes. While Octal DDR mode has 2 byte opcode and 4 byte address
phase, HF has 6 byte combined cmd-addr phase.

There is no support for Octal DDR flash currently. But there have been
multiple attempts to add Octal DDR mode support though:

https://patchwork.ozlabs.org/patch/982913/
https://lkml.org/lkml/2019/11/15/254
https://patchwork.ozlabs.org/patch/1236285/


>> So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
>> driver (instead of driver/memory) and use extended spi_mem_op to support
>> HyperFlash.
> 
>    I don't think cramming support for the different flash busses into
> the SPI drivers is a good idea... I'm not against generalizing the
> drivers/memory/ APIs though.
> 

IMO, its easier to extend spi-mem to support HF by adding an additional
field to indicate the protocol than creating a new one.
But, I am open to other generic ways to support these controllers as well.

>> [1]
>> https://ip.cadence.com/uploads/1244/cdn-dsd-mem-fla-host-controller-ip-for-xspi-pdf
> 
>    Do they have the full datasheet available? I'll try looking at the driver 
> tomorrow...
> 

I don't see a datasheet, you could probably ask on the patch adding the
driver (using the mbox from here:
https://patchwork.kernel.org/cover/11354193/). But above document
indicates its supports both the flashes.

Also have look at JEDEC xSPI spec that combines Octal DDR and HF into a
single standard: https://www.jedec.org/standards-documents/docs/jesd251

So, I expect more controllers to support SPI/QSPI + Octal DDR SPI + HF
with a single IP.

On Wed, 19 Feb 2020 23:13:36 +0300
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

> > One way would be to extend spi_mem_op to support above template along
> > with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
> > core can then register a spi_device and use spi-mem ops to talk to
> > controller driver.  
> 
>    We have discussed this idea with Mark Brown, the SPI maintainer, and
> he wasn't terribly impressed (I've invited him to #mtd -- his nick is
> broonie and mine is headless, I'm also adding him to CC:).
> 
> > So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
> > driver (instead of driver/memory) and use extended spi_mem_op to support
> > HyperFlash.  
> 
>    I don't think cramming support for the different flash busses into
> the SPI drivers is a good idea...

That's what I thought at first (SPI and Hyperflash seemed different
enough to not merge them), then I had a look at Vignesh's HyperFlash
presentation [1], and there's one aspect that made me reconsider this
PoV. Slide 25 (xSPI compliant HyperFlash): having devices bouncing from
one driver to another depending on the mode they operate in is likely to
be painful to handle. Not to mention that Octo+DTR is similar to
HyperBus from an HW PoV (at the PHY level, they both have CS, CLK,
DQS/RWDS, DQ/IO[0:7] signals, only the protocol differs).

[1]https://elinux.org/images/3/38/HBMC-v1.pdf

On Thu, 20 Feb 2020 08:46:23 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Wed, 19 Feb 2020 23:13:36 +0300
> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:
> 
> > > One way would be to extend spi_mem_op to support above template along
> > > with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
> > > core can then register a spi_device and use spi-mem ops to talk to
> > > controller driver.    
> > 
> >    We have discussed this idea with Mark Brown, the SPI maintainer, and
> > he wasn't terribly impressed (I've invited him to #mtd -- his nick is
> > broonie and mine is headless, I'm also adding him to CC:).
> >   
> > > So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
> > > driver (instead of driver/memory) and use extended spi_mem_op to support
> > > HyperFlash.    
> > 
> >    I don't think cramming support for the different flash busses into
> > the SPI drivers is a good idea...  
> 
> That's what I thought at first (SPI and Hyperflash seemed different
> enough to not merge them), then I had a look at Vignesh's HyperFlash
> presentation [1], and there's one aspect that made me reconsider this
> PoV. Slide 25 (xSPI compliant HyperFlash): having devices bouncing from
> one driver to another depending on the mode they operate in is likely to
> be painful to handle. Not to mention that Octo+DTR is similar to
> HyperBus from an HW PoV (at the PHY level, they both have CS, CLK,
> DQS/RWDS, DQ/IO[0:7] signals, only the protocol differs).

This doc [2] also shows the similarities between HyperBus and
Octal+DTR-SPI.

> 
> [1]https://elinux.org/images/3/38/HBMC-v1.pdf

[2]https://www.st.com/content/ccc/resource/technical/document/application_note/group0/91/dd/af/52/e1/d3/48/8e/DM00407776/files/DM00407776.pdf/jcr:content/translations/en.DM00407776.pdf

On 02/18/2020 02:11 PM, Vignesh Raghavendra wrote:

[...]
>>> Looking around, there seems to be more than one SPI controllers, apart
>>> from Renesas, which also support SPI NOR and HyperFlash protocol within
>>> a single IP block. E.g.: Cadence xSPI controller [1]. Therefore, we need
>>> a generic framework to support these kind of controllers.
>>>
>>> One way would be to extend spi_mem_op to support above template along
>>> with a new field to distinguish SPI NOR vs HyperFlash protocol. HyperBus
>>> core can then register a spi_device and use spi-mem ops to talk to
>>> controller driver.
>>> So, I suggest making Renesas RPC-IF backend a full fledged spi-mem
>>> driver (instead of driver/memory) and use extended spi_mem_op to support
>>> HyperFlash.
>>
>>
>> From Renesas Hyperflash user point of view, I wonder if a two step
>> approach would be possible and acceptable, here?
>>
>> Being a user of the Renesas Hyperflash, I want a driver for that. And,
>> of course, I want it "now" ;)
>>
>> So I wonder if it would be a valid option to have a functioning Renesas
>> Hypeflash driver, first. And in a second step abstract that in a more
>> generic way to support additional controllers. While in parallel having
>> a functional driver for the Renesas people, already.
> 
> AFAICS, the backend driver is not merged and is still in RFC phase.

   It was still marked RFC back in December and I haven't received any
feedback since, other than Dirk's request. Where have you been? Well,
I should have CCed linux-mtd back then... :-/

> Therefore I don't see any benefit of two step approach here. Besides
> you'll have to throw away this new driver (hyperbus/rpc-if.c) entirely
> later on.

   Why did you create this directory for, anyway? :-/

> How difficult is it to rewrite backend to be spi-mem driver? There is
> already has a spi_mem_ops frontend implementation, so I don't see much
> of an issue.

   Really? This may be not much of an issue with coding this but it's
certainly time consuming (I'm sure there's s/th to think about yet in
this case)? My management (and also me, so far) believes I'm in the
final stage with these drivers... what should I say to my boss now?

> Extending hyperbus core to use spi-mem should also straight forward
> Would involve moving this patch into core file.

   Seriously, only "moving"?

>> Is the support for [1] a more or less theoretical one, at the moment? Or
>> are there users of that which need support "now", too?
> 
> Its not theoretical, I do see patches for xSPI controller here:
> https://patchwork.kernel.org/cover/11354193/

   Which (surprise!) only adds support for the SPI part...

> So, its best to sort this out now so as to avoid possible backward
> compatibility issues (especially with DT bindings)

   What DT issues do you mean exactly? I think that other than changing
the "home" dir for the bindings, there'd little to change. The "front ends"
don't deal with the DT probing...

> Regards
> Vignesh

MBR, Sergei

[...]

>>> So I wonder if it would be a valid option to have a functioning Renesas
>>> Hypeflash driver, first. And in a second step abstract that in a more
>>> generic way to support additional controllers. While in parallel having
>>> a functional driver for the Renesas people, already.
>>
>> AFAICS, the backend driver is not merged and is still in RFC phase.
> 
>    It was still marked RFC back in December and I haven't received any
> feedback since, other than Dirk's request. Where have you been? Well,
> I should have CCed linux-mtd back then... :-/
> 

Well, as you said, this should have been discussed in linux-mtd :-/ And
therefore why my first question on this thread was where is the backend
driver.


>> Therefore I don't see any benefit of two step approach here. Besides
>> you'll have to throw away this new driver (hyperbus/rpc-if.c) entirely
>> later on.
> 
>    Why did you create this directory for, anyway? :-/
> 


This directory is not just for HyperBus controllers but also for flash
drivers.
While HF uses CFI command set, its not necessary that other HyperBus
memory devices do. For example HyperRAM would need a separate driver
which would be under this directory. Even, HF would need a translation
layer from map_* APIs for controllers that can't use map_ APIs directly
and to add HF only features

Then there is need to support HF only controllers such as TI's
HyperBus controller which does not support any of the SPI modes or full
xSPI specification but just initial HyperBus protocol.
I don't think drivers/spi is the right place for such controllers.


>> How difficult is it to rewrite backend to be spi-mem driver? There is
>> already has a spi_mem_ops frontend implementation, so I don't see much
>> of an issue.
> 
>    Really? This may be not much of an issue with coding this but it's
> certainly time consuming (I'm sure there's s/th to think about yet in
> this case)? My management (and also me, so far) believes I'm in the
> final stage with these drivers... what should I say to my boss now?
> 

That's is not my concern and above statements on ML won't help either..
Lets have a constructive discussion and come up with solution is that
maintainable in long term :-/

My aim was to explore whether its possible to support OSPI and HF using
a single driver without needing two frontend drivers and avoid switching
b/w two drivers when supporting xSPI compliant HFs

Again, I did not insist on extending spi_mem_op to be the _only_
option. I said we should have a generic framework to support controllers
such as RPC-IF which supports both OSPI and HF and one of the options is
to extend spi_mem_op.

And I am not responsible for RPC-IF series to go to v19 :(

>> Extending hyperbus core to use spi-mem should also straight forward
>> Would involve moving this patch into core file.
> 
>    Seriously, only "moving"?
> 


HyperBus framework is pretty small and can be refractored quite easily
to support spi_mem_op extension

>>> Is the support for [1] a more or less theoretical one, at the moment? Or
>>> are there users of that which need support "now", too?
>>
>> Its not theoretical, I do see patches for xSPI controller here:
>> https://patchwork.kernel.org/cover/11354193/
> 
>    Which (surprise!) only adds support for the SPI part...
> 
>> So, its best to sort this out now so as to avoid possible backward
>> compatibility issues (especially with DT bindings)
> 
>    What DT issues do you mean exactly? I think that other than changing
> the "home" dir for the bindings, there'd little to change. The "front ends"
> don't deal with the DT probing...
> 

OK, if there are no DT bindings for each of the frontend drivers then
this should not be concern.

Since, Mark seems okay with current approach (per IRC discussions)
and is not keen on extending spi_mem_ops, I will look at this patch as is.

PS: I cannot reply to you on IRC as I am on the opposite end of timezone.

--
Regards
Vignesh