Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RFT 0/2] Add RPC-IF HyperFlash driver
@ 2020-01-29 20:32 Sergei Shtylyov
  2020-01-29 20:37 ` [PATCH RFT 1/2] mtd: hyperbus: move direct mapping setup to AM654 HBMC driver Sergei Shtylyov
  2020-01-29 20:39 ` [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver Sergei Shtylyov
  0 siblings, 2 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-01-29 20:32 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd
  Cc: Dirk Behme

Hello!

Here's a set of 2 patches against the 'mtd/next' branch of the MTD 'linux.git' repo.
I'm adding the new HyperBus driver for Renesas Reduced Pin Count Interface (RPC-IF),
it's a "front-end" driver using the main, "back end" driver (from drivers/memory/)
that talks to the hardware. Unfortunately, the HyperBus core posed an obstacle to
doing this, as it assumes too much about the hardware's capabilities, so I had to
move the direct mapping setup code from the core to the TI AM654 driver...

[1] https://patchwork.kernel.org/patch/11283127/

[1/2] mtd: hyperbus: move direct mapping setup to AM654 HBMC driver
[2/2] mtd: hyperbus: add Renesas RPC-IF driver

MBR, Sergei

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH RFT 1/2] mtd: hyperbus: move direct mapping setup to AM654 HBMC driver
  2020-01-29 20:32 [PATCH RFT 0/2] Add RPC-IF HyperFlash driver Sergei Shtylyov
@ 2020-01-29 20:37 ` Sergei Shtylyov
  2020-01-29 20:39 ` [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver Sergei Shtylyov
  1 sibling, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-01-29 20:37 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd
  Cc: Dirk Behme

The Hyperbus core expects that HyperFlash is always directly mapped for
both read and write, but in reality this may not always be the case, e.g.
Renesas RPC-IF has read only direct mapping. Move the code setting up the
direct mapping from the Hyperbus core to thh TI AM554 HBMC driver.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/mtd/hyperbus/hbmc-am654.c    |   12 ++++++++++++
 drivers/mtd/hyperbus/hyperbus-core.c |   11 -----------
 2 files changed, 12 insertions(+), 11 deletions(-)

Index: linux/drivers/mtd/hyperbus/hbmc-am654.c
===================================================================
--- linux.orig/drivers/mtd/hyperbus/hbmc-am654.c
+++ linux/drivers/mtd/hyperbus/hbmc-am654.c
@@ -11,6 +11,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mux/consumer.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
@@ -57,8 +58,10 @@ static const struct hyperbus_ops am654_h
 
 static int am654_hbmc_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
 	struct am654_hbmc_priv *priv;
+	struct resource res;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -67,6 +70,10 @@ static int am654_hbmc_probe(struct platf
 
 	platform_set_drvdata(pdev, priv);
 
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret)
+		return ret;
+
 	if (of_property_read_bool(dev->of_node, "mux-controls")) {
 		struct mux_control *control = devm_mux_control_get(dev, NULL);
 
@@ -88,6 +95,11 @@ static int am654_hbmc_probe(struct platf
 		goto disable_pm;
 	}
 
+	priv->hbdev.map.size = resource_size(&res);
+	priv->hbdev.map.virt = devm_ioremap_resource(dev, &res);
+	if (IS_ERR(priv->hbdev.map.virt))
+		return PTR_ERR(priv->hbdev.map.virt);
+
 	priv->ctlr.dev = dev;
 	priv->ctlr.ops = &am654_hbmc_ops;
 	priv->hbdev.ctlr = &priv->ctlr;
Index: linux/drivers/mtd/hyperbus/hyperbus-core.c
===================================================================
--- linux.orig/drivers/mtd/hyperbus/hyperbus-core.c
+++ linux/drivers/mtd/hyperbus/hyperbus-core.c
@@ -10,7 +10,6 @@
 #include <linux/mtd/map.h>
 #include <linux/mtd/mtd.h>
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include <linux/types.h>
 
 static struct hyperbus_device *map_to_hbdev(struct map_info *map)
@@ -62,7 +61,6 @@ int hyperbus_register_device(struct hype
 	struct hyperbus_ctlr *ctlr;
 	struct device_node *np;
 	struct map_info *map;
-	struct resource res;
 	struct device *dev;
 	int ret;
 
@@ -78,17 +76,8 @@ int hyperbus_register_device(struct hype
 
 	hbdev->memtype = HYPERFLASH;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret)
-		return ret;
-
 	dev = ctlr->dev;
 	map = &hbdev->map;
-	map->size = resource_size(&res);
-	map->virt = devm_ioremap_resource(dev, &res);
-	if (IS_ERR(map->virt))
-		return PTR_ERR(map->virt);
-
 	map->name = dev_name(dev);
 	map->bankwidth = 2;
 	map->device_node = np;

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-01-29 20:32 [PATCH RFT 0/2] Add RPC-IF HyperFlash driver Sergei Shtylyov
  2020-01-29 20:37 ` [PATCH RFT 1/2] mtd: hyperbus: move direct mapping setup to AM654 HBMC driver Sergei Shtylyov
@ 2020-01-29 20:39 ` Sergei Shtylyov
  2020-02-03  4:59   ` Vignesh Raghavendra
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-01-29 20:39 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd
  Cc: Dirk Behme

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);
+	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");

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-01-29 20:39 ` [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver Sergei Shtylyov
@ 2020-02-03  4:59   ` Vignesh Raghavendra
  2020-02-03 11:46     ` Sergei Shtylyov
  2020-02-07 12:59   ` Behme Dirk (CM/ESO2)
  2020-02-18  4:00   ` Vignesh Raghavendra
  2 siblings, 1 reply; 13+ messages in thread
From: Vignesh Raghavendra @ 2020-02-03  4:59 UTC (permalink / raw)
  To: Sergei Shtylyov, Miquel Raynal, Richard Weinberger, linux-mtd; +Cc: Dirk Behme

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");
> 

-- 
Regards
Vignesh

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-03  4:59   ` Vignesh Raghavendra
@ 2020-02-03 11:46     ` Sergei Shtylyov
  0 siblings, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-02-03 11:46 UTC (permalink / raw)
  To: Vignesh Raghavendra, Miquel Raynal, Richard Weinberger, linux-mtd
  Cc: Dirk Behme

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-01-29 20:39 ` [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver Sergei Shtylyov
  2020-02-03  4:59   ` Vignesh Raghavendra
@ 2020-02-07 12:59   ` Behme Dirk (CM/ESO2)
  2020-02-07 19:09     ` Sergei Shtylyov
  2020-02-18  4:00   ` Vignesh Raghavendra
  2 siblings, 1 reply; 13+ messages in thread
From: Behme Dirk (CM/ESO2) @ 2020-02-07 12:59 UTC (permalink / raw)
  To: Sergei Shtylyov, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd

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



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-07 12:59   ` Behme Dirk (CM/ESO2)
@ 2020-02-07 19:09     ` Sergei Shtylyov
  2020-02-07 19:31       ` Dirk Behme
  0 siblings, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2020-02-07 19:09 UTC (permalink / raw)
  To: Behme Dirk (CM/ESO2),
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd

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


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-07 19:09     ` Sergei Shtylyov
@ 2020-02-07 19:31       ` Dirk Behme
  2020-02-07 20:17         ` Sergei Shtylyov
  0 siblings, 1 reply; 13+ messages in thread
From: Dirk Behme @ 2020-02-07 19:31 UTC (permalink / raw)
  To: Sergei Shtylyov, Behme Dirk (CM/ESO2),
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd
  Cc: Dirk Behme

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


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-07 19:31       ` Dirk Behme
@ 2020-02-07 20:17         ` Sergei Shtylyov
  2020-02-10  9:18           ` Behme Dirk (CM/ESO2)
  0 siblings, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2020-02-07 20:17 UTC (permalink / raw)
  To: Dirk Behme, Behme Dirk (CM/ESO2),
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-07 20:17         ` Sergei Shtylyov
@ 2020-02-10  9:18           ` Behme Dirk (CM/ESO2)
  0 siblings, 0 replies; 13+ messages in thread
From: Behme Dirk (CM/ESO2) @ 2020-02-10  9:18 UTC (permalink / raw)
  To: Sergei Shtylyov, Dirk Behme, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd

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);
  }
-- 
2.20.0



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-01-29 20:39 ` [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver Sergei Shtylyov
  2020-02-03  4:59   ` Vignesh Raghavendra
  2020-02-07 12:59   ` Behme Dirk (CM/ESO2)
@ 2020-02-18  4:00   ` Vignesh Raghavendra
  2020-02-18  7:12     ` Behme Dirk (CM/ESO2)
  2 siblings, 1 reply; 13+ messages in thread
From: Vignesh Raghavendra @ 2020-02-18  4:00 UTC (permalink / raw)
  To: Sergei Shtylyov, Miquel Raynal, Richard Weinberger, linux-mtd; +Cc: Dirk Behme

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");
> 

-- 
Regards
Vignesh

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-18  4:00   ` Vignesh Raghavendra
@ 2020-02-18  7:12     ` Behme Dirk (CM/ESO2)
  2020-02-18 11:11       ` Vignesh Raghavendra
  0 siblings, 1 reply; 13+ messages in thread
From: Behme Dirk (CM/ESO2) @ 2020-02-18  7:12 UTC (permalink / raw)
  To: Vignesh Raghavendra, Sergei Shtylyov, Miquel Raynal,
	Richard Weinberger, linux-mtd

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");

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver
  2020-02-18  7:12     ` Behme Dirk (CM/ESO2)
@ 2020-02-18 11:11       ` Vignesh Raghavendra
  0 siblings, 0 replies; 13+ messages in thread
From: Vignesh Raghavendra @ 2020-02-18 11:11 UTC (permalink / raw)
  To: Behme Dirk (CM/ESO2),
	Sergei Shtylyov, Miquel Raynal, Richard Weinberger, linux-mtd

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



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-29 20:32 [PATCH RFT 0/2] Add RPC-IF HyperFlash driver Sergei Shtylyov
2020-01-29 20:37 ` [PATCH RFT 1/2] mtd: hyperbus: move direct mapping setup to AM654 HBMC driver Sergei Shtylyov
2020-01-29 20:39 ` [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver Sergei Shtylyov
2020-02-03  4:59   ` Vignesh Raghavendra
2020-02-03 11:46     ` Sergei Shtylyov
2020-02-07 12:59   ` Behme Dirk (CM/ESO2)
2020-02-07 19:09     ` Sergei Shtylyov
2020-02-07 19:31       ` Dirk Behme
2020-02-07 20:17         ` Sergei Shtylyov
2020-02-10  9:18           ` Behme Dirk (CM/ESO2)
2020-02-18  4:00   ` Vignesh Raghavendra
2020-02-18  7:12     ` Behme Dirk (CM/ESO2)
2020-02-18 11:11       ` Vignesh Raghavendra

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git