Message ID | eba43289-3cb2-406b-cc5f-1209778621bf@cogentembedded.com |
---|---|
State | Changes Requested |
Delegated to: | Vignesh R |
Headers | show |
Series | Add RPC-IF HyperFlash driver | expand |
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
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");
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(+)