LinuxLists.cc - [RFC PATCH 0/5] MTD: Add Initial Hyperbus support

2019-02-19 06:37:56

Subject: [RFC PATCH 0/5] MTD: Add Initial Hyperbus support

2019-02-19 06:36:53

Subject: [RFC PATCH 5/5] mtd: hyperbus: Add driver for TI's Hyperbus memory controller

Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
IP is pretty simple and provides direct memory mapped access to
connected Flash devices.

Add basic support for the IP without DMA. Second ChipSelect is not
supported for now.

Signed-off-by: Vignesh R <[email protected]>
---
drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c

diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
new file mode 100644
index 000000000000..1f0d2dc52f9f
--- /dev/null
+++ b/drivers/mtd/hyperbus/hbmc_am654.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+// Author: Vignesh R <[email protected]>
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/hyperbus.h>
+#include <linux/mtd/mtd.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/types.h>
+
+struct am654_hbmc_priv {
+ struct hb_device hbdev;
+ void __iomem *regbase;
+};
+
+static int am654_hbmc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct am654_hbmc_priv *priv;
+ struct resource *res;
+ int err;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (IS_ERR(res)) {
+ dev_err(&pdev->dev, "failed to get memory resource\n");
+ return -ENOENT;
+ }
+
+ priv->regbase = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->regbase)) {
+ dev_err(dev, "Cannot remap controller address.\n");
+ return PTR_ERR(priv->regbase);
+ }
+
+ pm_runtime_enable(&pdev->dev);
+ err = pm_runtime_get_sync(&pdev->dev);
+ if (err < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ return err;
+ }
+
+ priv->hbdev.needs_calib = true;
+ priv->hbdev.dev = &pdev->dev;
+ priv->hbdev.np = of_get_next_child(dev->of_node, NULL);
+ err = hb_register_device(&priv->hbdev);
+ if (err) {
+ dev_err(&pdev->dev, "failed to register controller\n");
+ goto err_destroy;
+ }
+
+ return 0;
+
+err_destroy:
+ hb_unregister_device(&priv->hbdev);
+ pm_runtime_put_sync(&pdev->dev);
+ return err;
+}
+
+static int am654_hbmc_remove(struct platform_device *pdev)
+{
+ struct am654_hbmc_priv *priv = platform_get_drvdata(pdev);
+ int ret;
+
+ ret = hb_unregister_device(&priv->hbdev);
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
+ return ret;
+}
+
+static const struct of_device_id am654_hbmc_dt_ids[] = {
+ {
+ .compatible = "ti,am654-hbmc",
+ },
+ { /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, am654_hbmc_dt_ids);
+
+static struct platform_driver am654_hbmc_platform_driver = {
+ .probe = am654_hbmc_probe,
+ .remove = am654_hbmc_remove,
+ .driver = {
+ .name = "hbmc-am654",
+ .of_match_table = am654_hbmc_dt_ids,
+ },
+};
+
+module_platform_driver(am654_hbmc_platform_driver);
+
+MODULE_DESCRIPTION("HBMC driver for AM654 SoC");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hbmc-am654");
+MODULE_AUTHOR("Vignesh R <[email protected]>");
--
2.20.1

2019-02-19 06:37:14

by Vignesh Raghavendra

[permalink] [raw]

Subject: [RFC PATCH 4/5] dt-bindings: mtd: Add bindings for TI's AM654 Hyperbus memory controller

Add binding documentation for TI's Hyperbus memory controller present on
AM654 SoC.

Signed-off-by: Vignesh R <[email protected]>
---
.../devicetree/bindings/mtd/ti,am654-hbmc.txt | 27 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt

diff --git a/Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt b/Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt
new file mode 100644
index 000000000000..b70fd1795fc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt
@@ -0,0 +1,27 @@
+Bindings for Hyperbus Memory Controller on TI's K3 family of SoCs
+
+Required properties:
+- compatible : "ti,am654-hbmc" for AM654 SoC
+- reg : Two entries:
+ First entry pointed to the register space of HBMC controller
+ Second entry pointing to the memory map region dedicated for
+ MMIO access to attached flash devices
+- ranges : Address range allocated for each Chipselect in the MMIO space
+
+Example:
+ hbmc: hbmc@47034000 {
+ compatible = "ti,am654-hbmc";
+ reg = <0x0 0x47034000 0x0 0x100>,
+ <0x5 0x00000000 0x1 0x0000000>;
+ power-domains = <&k3_pds 55>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x5 0x00000000 0x4000000>, /* CS0 - 64MB */
+ <0x1 0x5 0x04000000 0x4000000>; /* CS1 - 64MB
+
+ /* Slave flash node */
+ flash@0{
+ compatible = "cypress,hyperflash";
+ reg = <0x0 0x4000000>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 0808c22807bc..a9760c5e33f7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7149,6 +7149,7 @@ S: Supported
F: drivers/mtd/hyperbus/
F: include/linux/mtd/hyperbus.h
F: Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt
+F: Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt

HYPERVISOR VIRTUAL CONSOLE DRIVER
L: [email protected]
--
2.20.1

2019-02-19 06:37:25

by Vignesh Raghavendra

[permalink] [raw]

Subject: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
interface between a host system master and one or more slave interfaces.
HyperBus is used to connect microprocessor, microcontroller, or ASIC
devices with random access NOR flash memory(called HyperFlash) or
self refresh DRAM(called HyperRAM).

Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
signal and either Single-ended clock(3.0V parts) or Differential clock
(1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
At bus level, it follows a separate protocol described in HyperBus
specification[1].

HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
operates at >166MHz frequencies.
HyperRAM provides direct random read/write access to flash memory
array.

But, Hyperbus memory controllers seem to abstract implementation details
and expose a simple MMIO interface to access connected flash.

Add support for registering HyperFlash devices with MTD framework. MTD
maps framework along with CFI chip support framework are used to support
communicate with flash.

Framework is modelled along the lines of spi-nor framework. HyperBus
memory controller(HBMC) drivers call hb_register_device() to register a
single HyperFlash device. HyperFlash core parses MMIO access
information from DT, sets up the map_info struct, probes CFI flash and
registers it with MTD framework.

Some HBMC masters need calibration/training sequence[3] to be carried
out, in order for DLL inside the controller to lock, by reading a known
string/pattern. This is done by repeatedly reading CFI Query
Identification String. Calibration needs to be done before try to detect
flash as part of CFI flash probe.

HyperRAM is not supported atm.

HyperBus specification can be found at[1]
HyperFlash datasheet can be found at[2]

[1] https://www.cypress.com/file/213356/download
[2] https://www.cypress.com/file/213346/download
[3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
Table 12-5741. HyperFlash Access Sequence

Signed-off-by: Vignesh R <[email protected]>
---
MAINTAINERS | 7 ++
drivers/mtd/Kconfig | 2 +
drivers/mtd/Makefile | 1 +
drivers/mtd/hyperbus/Kconfig | 23 +++++
drivers/mtd/hyperbus/Makefile | 4 +
drivers/mtd/hyperbus/core.c | 167 ++++++++++++++++++++++++++++++++++
include/linux/mtd/hyperbus.h | 73 +++++++++++++++
7 files changed, 277 insertions(+)
create mode 100644 drivers/mtd/hyperbus/Kconfig
create mode 100644 drivers/mtd/hyperbus/Makefile
create mode 100644 drivers/mtd/hyperbus/core.c
create mode 100644 include/linux/mtd/hyperbus.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 08bf7418a97f..0808c22807bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7143,6 +7143,13 @@ F: include/uapi/linux/hyperv.h
F: tools/hv/
F: Documentation/ABI/stable/sysfs-bus-vmbus

+HYPERBUS SUPPORT
+M: Vignesh R <[email protected]>
+S: Supported
+F: drivers/mtd/hyperbus/
+F: include/linux/mtd/hyperbus.h
+F: Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt
+
HYPERVISOR VIRTUAL CONSOLE DRIVER
L: [email protected]
S: Odd Fixes
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 79a8ff542883..a915ff300390 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -290,4 +290,6 @@ source "drivers/mtd/spi-nor/Kconfig"

source "drivers/mtd/ubi/Kconfig"

+source "drivers/mtd/hyperbus/Kconfig"
+
endif # MTD
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 58fc327a5276..04c154906631 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -35,3 +35,4 @@ obj-y += chips/ lpddr/ maps/ devices/ nand/ tests/

obj-$(CONFIG_MTD_SPI_NOR) += spi-nor/
obj-$(CONFIG_MTD_UBI) += ubi/
+obj-$(CONFIG_MTD_HYPERBUS) += hyperbus/
diff --git a/drivers/mtd/hyperbus/Kconfig b/drivers/mtd/hyperbus/Kconfig
new file mode 100644
index 000000000000..02c38afc5c50
--- /dev/null
+++ b/drivers/mtd/hyperbus/Kconfig
@@ -0,0 +1,23 @@
+menuconfig MTD_HYPERBUS
+ tristate "Hyperbus support"
+ select MTD_CFI
+ select MTD_MAP_BANK_WIDTH_2
+ select MTD_CFI_AMDSTD
+ select MTD_COMPLEX_MAPPINGS
+ help
+ This is the framework for the Hyperbus which can be used by
+ the Hyperbus Controller driver to commmunicate with
+ Hyperflash. See Cypress Hyperbus specification for more
+ details
+
+
+if MTD_HYPERBUS
+
+config HBMC_AM654
+ tristate "Hyperbus controller driver for AM65x SoC"
+ help
+ This is the driver for Hyperbus controller on TI's AM65x and
+ other SoCs
+
+endif # MTD_HYPERBUS
+
diff --git a/drivers/mtd/hyperbus/Makefile b/drivers/mtd/hyperbus/Makefile
new file mode 100644
index 000000000000..64e377d7f636
--- /dev/null
+++ b/drivers/mtd/hyperbus/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_MTD_HYPERBUS) += core.o
+obj-$(CONFIG_HBMC_AM654) += hbmc_am654.o
diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
new file mode 100644
index 000000000000..d3d44aab7503
--- /dev/null
+++ b/drivers/mtd/hyperbus/core.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+// Author: Vignesh R <[email protected]>
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/mtd/hyperbus.h>
+#include <linux/mtd/map.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/cfi.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/types.h>
+
+#define HB_CALIB_COUNT 25
+
+static struct hb_device *map_to_hbdev(struct map_info *map)
+{
+ return container_of(map, struct hb_device, map);
+}
+
+static map_word hb_read16(struct map_info *map, unsigned long addr)
+{
+ struct hb_device *hbdev = map_to_hbdev(map);
+ map_word read_data;
+
+ read_data.x[0] = hbdev->ops->read16(hbdev, addr);
+
+ return read_data;
+}
+
+static void hb_write16(struct map_info *map, map_word d, unsigned long addr)
+{
+ struct hb_device *hbdev = map_to_hbdev(map);
+
+ hbdev->ops->write16(hbdev, addr, d.x[0]);
+}
+
+static void hb_copy_from(struct map_info *map, void *to,
+ unsigned long from, ssize_t len)
+{
+ struct hb_device *hbdev = map_to_hbdev(map);
+
+ hbdev->ops->copy_from(hbdev, to, from, len);
+}
+
+static void hb_copy_to(struct map_info *map, unsigned long to,
+ const void *from, ssize_t len)
+{
+ struct hb_device *hbdev = map_to_hbdev(map);
+
+ hbdev->ops->copy_to(hbdev, to, from, len);
+}
+
+/* Calibrate HBMC by repeatedly reading "QRY" string from CFI space */
+static int hb_calibrate(struct hb_device *hbdev)
+{
+ struct map_info *map = &hbdev->map;
+ struct cfi_private cfi;
+ int count = HB_CALIB_COUNT;
+ int ret;
+
+ cfi.interleave = 1;
+ cfi.device_type = CFI_DEVICETYPE_X16;
+ cfi_send_gen_cmd(0xF0, 0, 0, map, &cfi, cfi.device_type, NULL);
+ cfi_send_gen_cmd(0x98, 0x55, 0, map, &cfi, cfi.device_type, NULL);
+
+ while (count--)
+ cfi_qry_present(map, 0, &cfi);
+
+ ret = cfi_qry_present(map, 0, &cfi);
+ cfi_qry_mode_off(0, map, &cfi);
+
+ return ret;
+}
+
+int hb_register_device(struct hb_device *hbdev)
+{
+ struct resource res;
+ struct device *dev;
+ struct device_node *np;
+ struct map_info *map;
+ struct hb_ops *ops;
+ int err;
+
+ if (!hbdev || !hbdev->dev || !hbdev->np) {
+ pr_err("hyperbus: please fill all the necessary fields!\n");
+ return -EINVAL;
+ }
+
+ np = hbdev->np;
+ if (!of_device_is_compatible(np, "cypress,hyperflash"))
+ return -ENODEV;
+
+ hbdev->memtype = HYPERFLASH;
+
+ if (of_address_to_resource(np, 0, &res))
+ return -EINVAL;
+
+ dev = hbdev->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;
+
+ simple_map_init(map);
+ ops = hbdev->ops;
+ if (ops) {
+ if (ops->read16)
+ map->read = hb_read16;
+ if (ops->write16)
+ map->write = hb_write16;
+ if (ops->copy_to)
+ map->copy_to = hb_copy_to;
+ if (ops->copy_from)
+ map->copy_from = hb_copy_from;
+ }
+
+ if (hbdev->needs_calib) {
+ err = hb_calibrate(hbdev);
+ if (!err) {
+ dev_err(hbdev->dev, "Calibration failed\n");
+ return -ENODEV;
+ }
+ }
+
+ hbdev->mtd = do_map_probe("cfi_probe", map);
+ if (!hbdev->mtd) {
+ dev_err(hbdev->dev, "probing failed\n");
+ return -ENXIO;
+ }
+
+ hbdev->mtd->dev.parent = hbdev->dev;
+ mtd_set_of_node(hbdev->mtd, np);
+
+ err = mtd_device_register(hbdev->mtd, NULL, 0);
+ if (err) {
+ dev_err(hbdev->dev, "failed to register mtd device\n");
+ goto err_destroy;
+ }
+ hbdev->registered = true;
+
+ return 0;
+
+err_destroy:
+ map_destroy(hbdev->mtd);
+ return err;
+}
+EXPORT_SYMBOL_GPL(hb_register_device);
+
+int hb_unregister_device(struct hb_device *hbdev)
+{
+ int ret = 0;
+
+ if (hbdev && hbdev->mtd && hbdev->registered) {
+ ret = mtd_device_unregister(hbdev->mtd);
+ map_destroy(hbdev->mtd);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(hb_unregister_device);
diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
new file mode 100644
index 000000000000..0aa11458c424
--- /dev/null
+++ b/include/linux/mtd/hyperbus.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef __LINUX_MTD_HYPERBUS_H__
+#define __LINUX_MTD_HYPERBUS_H__
+
+#include <linux/mtd/map.h>
+
+enum hb_memtype {
+ HYPERFLASH,
+ HYPERRAM,
+};
+
+/**
+ * struct hb_device - struct representing Hyperbus slave device
+ * @map: map_info struct for accessing MMIO Hyperbus flash memory
+ * @dev: device pointer of Hyperbus Controller
+ * @np: pointer to Hyperbus slave device node
+ * @mtd: pointer to MTD struct
+ * @ops: pointer to custom Hyperbus ops
+ * @memtype: type of memory device: Hyperflash or HyperRAM
+ * @needs_calib: flag to indicate whether calibration sequence is needed
+ * @registered: flag to indicate whether device is registered with MTD core
+ */
+
+struct hb_device {
+ struct map_info map;
+ struct device *dev;
+ struct device_node *np;
+ struct mtd_info *mtd;
+ struct hb_ops *ops;
+ enum hb_memtype memtype;
+ bool needs_calib;
+ bool registered;
+};
+
+/**
+ * struct hb_ops - struct representing custom Hyperbus operations
+ * @read16: read 16 bit of data, usually from register/ID-CFI space
+ * @write16: write 16 bit of data, usually to register/ID-CFI space
+ * copy_from: copy data from flash memory
+ * copy_to: copy data to flash_memory
+ */
+
+struct hb_ops {
+ u16 (*read16)(struct hb_device *hbdev, unsigned long addr);
+ void (*write16)(struct hb_device *hbdev, unsigned long addr, u16 val);
+
+ void (*copy_from)(struct hb_device *hbdev, void *to,
+ unsigned long from, ssize_t len);
+ void (*copy_to)(struct hb_device *dev, unsigned long to,
+ const void *from, ssize_t len);
+};
+
+/**
+ * hb_register_device - probe and register a Hyperbus slave memory device
+ * @hbdev: hb_device struct with dev, np populated
+ *
+ * Return: 0 for success, others for failure.
+ */
+int hb_register_device(struct hb_device *hbdev);
+
+/**
+ * hb_unregister_device - deregister Hyperbus slave memory device
+ * @hbdev: hb_device struct of device to be unregistered
+ *
+ * Return: 0 for success, others for failure.
+ */
+int hb_unregister_device(struct hb_device *hbdev);
+
+#endif /* __LINUX_MTD_HYPERBUS_H__ */
--
2.20.1

2019-02-19 06:37:41

by Vignesh Raghavendra

[permalink] [raw]

Subject: [RFC PATCH 2/5] dt-bindings: mtd: Add binding documentation for Hyperbus memory devices

Add DT binding documentation for Hyperbus memory devices. For now only
Hyperflash is supported.

Signed-off-by: Vignesh R <[email protected]>
---
Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt | 6 ++++++
1 file changed, 6 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt

diff --git a/Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt b/Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt
new file mode 100644
index 000000000000..cb408f80b868
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt
@@ -0,0 +1,6 @@
+Bindings for Hyperflash NOR flash chips compliant with Cypress Hyperbus
+specification and supports Cypress CFI specification 1.5 command set.
+
+Required properties:
+- compatible : "cypress,hyperflash" for Hyperflash NOR chips
+- reg : Address where flash's memory mapped space
--
2.20.1

2019-02-19 06:38:45

by Vignesh Raghavendra

[permalink] [raw]

Subject: [RFC PATCH 1/5] mtd: cfi_cmdset_0002: Add support for polling status register

HyperFlash devices are compliant with CFI AMD/Fujitsu Extended Command
Set(0x0002) for flash operations, therefore drivers/mtd/chips/cfi_cmdset_0002.c
can be use as is. But these devices do not support DQ polling method of
determining chip ready/good status. These flashes provide Status
Register whose bits can be polled to know status of flash operation.

Cypress HyperFlash datasheet here[1], talks about CFI Amd/Fujitsu
Extended Query version 1.5. Bit 0 of "Software Features supported" field
of CFI Primary Vendor-Specific Extended Query table indicates
presence/absence of status register and Bit 1 indicates whether or not
DQ polling is supported. Using these bits, its possible to determine
whether flash supports DQ polling or need to use Status Register.

Add support for polling status register to know device ready/status of
erase/write operations when DQ polling is not supported.

[1] https://www.cypress.com/file/213346/download

Signed-off-by: Vignesh R <[email protected]>
---

Note: PRI extended query table size is bigger on 1.5 than on older
revision. Not sure if this causes problems on older rev. because of
reading beyond current size.

drivers/mtd/chips/cfi_cmdset_0002.c | 50 +++++++++++++++++++++++++++++
include/linux/mtd/cfi.h | 5 +++
2 files changed, 55 insertions(+)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 72428b6bfc47..7a4ef0237750 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -49,6 +49,14 @@
#define SST49LF008A 0x005a
#define AT49BV6416 0x00d6

+/*
+ * Bits of Status Register definition for flash devices that don't
+ * support DQ polling (Eg.: Hyperflash)
+ */
+#define CFI_SR_DRB BIT(7)
+#define CFI_SR_ESB BIT(5)
+#define CFI_SR_PSB BIT(4)
+
static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
static int cfi_amdstd_write_buffers(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
@@ -97,6 +105,18 @@ static struct mtd_chip_driver cfi_amdstd_chipdrv = {
.module = THIS_MODULE
};

+/*
+ * Use status register to poll for Erase/write completion when DQ is not
+ * supported. This is indicated by Bit[1:0] of SoftwareFeatures field in
+ * CFI Primary Vendor-Specific Extended Query table 1.5
+ */
+static int cfi_use_status_reg(struct cfi_private *cfi)
+{
+ struct cfi_pri_amdstd *extp = cfi->cmdset_priv;
+
+ return (extp->MinorVersion >= '5') &&
+ ((extp->SoftwareFeatures & 0x11) == 1);
+}

/* #define DEBUG_CFI_FEATURES */

@@ -744,8 +764,21 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
*/
static int __xipram chip_ready(struct map_info *map, unsigned long addr)
{
+ struct cfi_private *cfi = map->fldrv_priv;
map_word d, t;

+ if (cfi_use_status_reg(cfi)) {
+ /*
+ * For chips that support status register, check device
+ * ready bit
+ */
+ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, 0, map, cfi,
+ cfi->device_type, NULL);
+ d = map_read(map, addr);
+
+ return (d.x[0] & CFI_SR_DRB);
+ }
+
d = map_read(map, addr);
t = map_read(map, addr);

@@ -769,8 +802,25 @@ static int __xipram chip_ready(struct map_info *map, unsigned long addr)
*/
static int __xipram chip_good(struct map_info *map, unsigned long addr, map_word expected)
{
+ struct cfi_private *cfi = map->fldrv_priv;
map_word oldd, curd;

+ if (cfi_use_status_reg(cfi)) {
+ /*
+ * For chips that support status register, check device
+ * ready bit and Erase/Program status bit to know if
+ * operation succeeded.
+ */
+ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, 0, map, cfi,
+ cfi->device_type, NULL);
+ curd = map_read(map, addr);
+
+ if (curd.x[0] & CFI_SR_DRB)
+ return !(curd.x[0] & (CFI_SR_PSB | CFI_SR_ESB));
+
+ return 0;
+ }
+
oldd = map_read(map, addr);
curd = map_read(map, addr);

diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index cbf77168658c..92ac82ac2329 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -233,6 +233,11 @@ struct cfi_pri_amdstd {
uint8_t VppMin;
uint8_t VppMax;
uint8_t TopBottom;
+ /* Below field are added from version 1.5 */
+ uint8_t ProgramSuspend;
+ uint8_t UnlockBypass;
+ uint8_t SecureSiliconSector;
+ uint8_t SoftwareFeatures;
} __packed;

/* Vendor-Specific PRI for Atmel chips (command set 0x0002) */
--
2.20.1

2019-02-19 07:25:15

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 0/5] MTD: Add Initial Hyperbus support

On 19/02/19 12:06 PM, Vignesh R wrote:
[...]
>
> Tested on modified TI AM654 EVM with Cypress Hyperflash S26KS512 by
> creating a UBIFS partition and writing and reading files to it.
> Stress tested by writing/reading 16MB flash repeatedly at different
> offsets using dd commmand.
>

Here is the enumeration log from dmesg:
https://pastebin.ubuntu.com/p/Vd5jYQT4FH/

> HyperBus specification can be found at[1]
> HyperFlash datasheet can be found at[2]
> TI's HBMC controller details at[3]
>
> [1] https://www.cypress.com/file/213356/download
> [2] https://www.cypress.com/file/213346/download
> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
> Table 12-5741. HyperFlash Access Sequence
>
> Vignesh R (5):
> mtd: cfi_cmdset_0002: Add support for polling status register
> dt-bindings: mtd: Add binding documentation for Hyperbus memory
> devices
> mtd: Add support for Hyperbus memory devices
> dt-bindings: mtd: Add bindings for TI's AM654 Hyperbus memory
> controller
> mtd: hyperbus: Add driver for TI's Hyperbus memory controller
>
> .../bindings/mtd/cypress,hyperbus.txt | 6 +
> .../devicetree/bindings/mtd/ti,am654-hbmc.txt | 27 +++
> MAINTAINERS | 8 +
> drivers/mtd/Kconfig | 2 +
> drivers/mtd/Makefile | 1 +
> drivers/mtd/chips/cfi_cmdset_0002.c | 50 ++++++
> drivers/mtd/hyperbus/Kconfig | 23 +++
> drivers/mtd/hyperbus/Makefile | 4 +
> drivers/mtd/hyperbus/core.c | 167 ++++++++++++++++++
> drivers/mtd/hyperbus/hbmc_am654.c | 105 +++++++++++
> include/linux/mtd/cfi.h | 5 +
> include/linux/mtd/hyperbus.h | 73 ++++++++
> 12 files changed, 471 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt
> create mode 100644 Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt
> create mode 100644 drivers/mtd/hyperbus/Kconfig
> create mode 100644 drivers/mtd/hyperbus/Makefile
> create mode 100644 drivers/mtd/hyperbus/core.c
> create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
> create mode 100644 include/linux/mtd/hyperbus.h
>

--
Regards
Vignesh

2019-02-19 07:54:37

by Boris Brezillon

[permalink] [raw]

Subject: Re: [RFC PATCH 0/5] MTD: Add Initial Hyperbus support

+Sergei and Mason who recently worked on an HyperFlash controller.

On Tue, 19 Feb 2019 12:06:02 +0530
Vignesh R <[email protected]> wrote:

> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
> interface between a host system master and one or more slave interfaces.
> HyperBus is used to connect microprocessor, microcontroller, or ASIC
> devices with random access NOR flash memory(called HyperFlash) or
> self refresh DRAM(called HyperRAM).
>
> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
> signal and either Single-ended clock(3.0V parts) or Differential clock
> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
> At bus level, it follows a separate protocol described in HyperBus
> specification[1].
>
> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
> operates at >166MHz frequencies.
> HyperRAM provides direct random read/write access to flash memory
> array.
> Framework is modelled along the lines of spi-nor framework. HyperBus
> memory controller(HBMC) drivers call hb_register_device() to register a
> single HyperFlash device. HyperFlash core parses MMIO access
> information from DT, sets up the map_info struct, probes CFI flash and
> registers it with MTD framework.
>
> This is an early RFC, to know if its okay to use maps framework and existing
> CFI compliant flash support code to support Hyperflash
> Also would like input on different types of HBMC master IPs out there
> and their programming sequences.
> Would appreciate any testing/review.
>
> Tested on modified TI AM654 EVM with Cypress Hyperflash S26KS512 by
> creating a UBIFS partition and writing and reading files to it.
> Stress tested by writing/reading 16MB flash repeatedly at different
> offsets using dd commmand.
>
> HyperBus specification can be found at[1]
> HyperFlash datasheet can be found at[2]
> TI's HBMC controller details at[3]
>
> [1] https://www.cypress.com/file/213356/download
> [2] https://www.cypress.com/file/213346/download
> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
> Table 12-5741. HyperFlash Access Sequence
>
> Vignesh R (5):
> mtd: cfi_cmdset_0002: Add support for polling status register
> dt-bindings: mtd: Add binding documentation for Hyperbus memory
> devices
> mtd: Add support for Hyperbus memory devices
> dt-bindings: mtd: Add bindings for TI's AM654 Hyperbus memory
> controller
> mtd: hyperbus: Add driver for TI's Hyperbus memory controller
>
> .../bindings/mtd/cypress,hyperbus.txt | 6 +
> .../devicetree/bindings/mtd/ti,am654-hbmc.txt | 27 +++
> MAINTAINERS | 8 +
> drivers/mtd/Kconfig | 2 +
> drivers/mtd/Makefile | 1 +
> drivers/mtd/chips/cfi_cmdset_0002.c | 50 ++++++
> drivers/mtd/hyperbus/Kconfig | 23 +++
> drivers/mtd/hyperbus/Makefile | 4 +
> drivers/mtd/hyperbus/core.c | 167 ++++++++++++++++++
> drivers/mtd/hyperbus/hbmc_am654.c | 105 +++++++++++
> include/linux/mtd/cfi.h | 5 +
> include/linux/mtd/hyperbus.h | 73 ++++++++
> 12 files changed, 471 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/cypress,hyperbus.txt
> create mode 100644 Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt
> create mode 100644 drivers/mtd/hyperbus/Kconfig
> create mode 100644 drivers/mtd/hyperbus/Makefile
> create mode 100644 drivers/mtd/hyperbus/core.c
> create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
> create mode 100644 include/linux/mtd/hyperbus.h
>

2019-02-23 18:41:52

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 1/5] mtd: cfi_cmdset_0002: Add support for polling status register

Hello!

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> HyperFlash devices are compliant with CFI AMD/Fujitsu Extended Command
> Set(0x0002) for flash operations, therefore drivers/mtd/chips/cfi_cmdset_0002.c
> can be use as is. But these devices do not support DQ polling method of
> determining chip ready/good status. These flashes provide Status
> Register whose bits can be polled to know status of flash operation.
>
> Cypress HyperFlash datasheet here[1], talks about CFI Amd/Fujitsu
> Extended Query version 1.5. Bit 0 of "Software Features supported" field
> of CFI Primary Vendor-Specific Extended Query table indicates
> presence/absence of status register and Bit 1 indicates whether or not
> DQ polling is supported. Using these bits, its possible to determine
> whether flash supports DQ polling or need to use Status Register.
>
> Add support for polling status register to know device ready/status of
> erase/write operations when DQ polling is not supported.
>
> [1] https://www.cypress.com/file/213346/download
>
> Signed-off-by: Vignesh R <[email protected]>
> ---
>
> Note: PRI extended query table size is bigger on 1.5 than on older
> revision. Not sure if this causes problems on older rev. because of
> reading beyond current size.
>
> drivers/mtd/chips/cfi_cmdset_0002.c | 50 +++++++++++++++++++++++++++++
> include/linux/mtd/cfi.h | 5 +++
> 2 files changed, 55 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 72428b6bfc47..7a4ef0237750 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
[...]
> @@ -97,6 +105,18 @@ static struct mtd_chip_driver cfi_amdstd_chipdrv = {
> .module = THIS_MODULE
> };
>
> +/*
> + * Use status register to poll for Erase/write completion when DQ is not
> + * supported. This is indicated by Bit[1:0] of SoftwareFeatures field in
> + * CFI Primary Vendor-Specific Extended Query table 1.5
> + */
> +static int cfi_use_status_reg(struct cfi_private *cfi)
> +{
> + struct cfi_pri_amdstd *extp = cfi->cmdset_priv;
> +
> + return (extp->MinorVersion >= '5') &&
> + ((extp->SoftwareFeatures & 0x11) == 1);

Parens not necessary here.

[...]
> @@ -744,8 +764,21 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
> */
> static int __xipram chip_ready(struct map_info *map, unsigned long addr)
> {
> + struct cfi_private *cfi = map->fldrv_priv;
> map_word d, t;
>
> + if (cfi_use_status_reg(cfi)) {
> + /*
> + * For chips that support status register, check device
> + * ready bit
> + */
> + cfi_send_gen_cmd(0x70, cfi->addr_unlock1, 0, map, cfi,
> + cfi->device_type, NULL);
> + d = map_read(map, addr);
> +
> + return (d.x[0] & CFI_SR_DRB);

Again, parens not needed.

[...]

Other than that, looks good.

MBR, Sergei

2019-02-23 18:44:51

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 1/5] mtd: cfi_cmdset_0002: Add support for polling status register

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> HyperFlash devices are compliant with CFI AMD/Fujitsu Extended Command
> Set(0x0002) for flash operations, therefore drivers/mtd/chips/cfi_cmdset_0002.c
> can be use as is. But these devices do not support DQ polling method of
> determining chip ready/good status. These flashes provide Status
> Register whose bits can be polled to know status of flash operation.
>
> Cypress HyperFlash datasheet here[1], talks about CFI Amd/Fujitsu
> Extended Query version 1.5. Bit 0 of "Software Features supported" field
> of CFI Primary Vendor-Specific Extended Query table indicates
> presence/absence of status register and Bit 1 indicates whether or not
> DQ polling is supported. Using these bits, its possible to determine
> whether flash supports DQ polling or need to use Status Register.
>
> Add support for polling status register to know device ready/status of
> erase/write operations when DQ polling is not supported.
>
> [1] https://www.cypress.com/file/213346/download
>
> Signed-off-by: Vignesh R <[email protected]>

Forgot about one thing: tags like S-o-b: need your full name,
cfr. Documentation/process/5.Posting.rst...

[...]

MBR, Sergei

2019-02-23 20:21:00

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

Hello!

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
> interface between a host system master and one or more slave interfaces.
> HyperBus is used to connect microprocessor, microcontroller, or ASIC
> devices with random access NOR flash memory(called HyperFlash) or

Need space before (.

> self refresh DRAM(called HyperRAM).
>
> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
> signal and either Single-ended clock(3.0V parts) or Differential clock
> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
> At bus level, it follows a separate protocol described in HyperBus
> specification[1].
>
> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
> operates at >166MHz frequencies.
> HyperRAM provides direct random read/write access to flash memory
> array.
>
> But, Hyperbus memory controllers seem to abstract implementation details
> and expose a simple MMIO interface to access connected flash.
>
> Add support for registering HyperFlash devices with MTD framework. MTD
> maps framework along with CFI chip support framework are used to support
> communicate with flash.

Communicating.

> Framework is modelled along the lines of spi-nor framework. HyperBus
> memory controller(HBMC) drivers call hb_register_device() to register a

Again, space needed before (.

> single HyperFlash device. HyperFlash core parses MMIO access
> information from DT, sets up the map_info struct, probes CFI flash and
> registers it with MTD framework.
>
> Some HBMC masters need calibration/training sequence[3] to be carried
> out, in order for DLL inside the controller to lock, by reading a known
> string/pattern. This is done by repeatedly reading CFI Query
> Identification String. Calibration needs to be done before try to detect

Trying.

> flash as part of CFI flash probe.
>
> HyperRAM is not supported atm.

ATM?

> HyperBus specification can be found at[1]
> HyperFlash datasheet can be found at[2]
>
> [1] https://www.cypress.com/file/213356/download
> [2] https://www.cypress.com/file/213346/download
> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
> Table 12-5741. HyperFlash Access Sequence
>
> Signed-off-by: Vignesh R <[email protected]>
[...]
> diff --git a/drivers/mtd/hyperbus/Kconfig b/drivers/mtd/hyperbus/Kconfig
> new file mode 100644
> index 000000000000..02c38afc5c50
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/Kconfig
> @@ -0,0 +1,23 @@
> +menuconfig MTD_HYPERBUS
> + tristate "Hyperbus support"
> + select MTD_CFI
> + select MTD_MAP_BANK_WIDTH_2
> + select MTD_CFI_AMDSTD
> + select MTD_COMPLEX_MAPPINGS
> + help
> + This is the framework for the Hyperbus which can be used by
> + the Hyperbus Controller driver to commmunicate with
^^^
Too many m's. :-)

> + Hyperflash. See Cypress Hyperbus specification for more
> + details
> +
> +
> +if MTD_HYPERBUS
> +
> +config HBMC_AM654
> + tristate "Hyperbus controller driver for AM65x SoC"
> + help
> + This is the driver for Hyperbus controller on TI's AM65x and
> + other SoCs
> +
> +endif # MTD_HYPERBUS

The above clearly belongs to patch #5.

> +

No empty lines at end of file, please...

> diff --git a/drivers/mtd/hyperbus/Makefile b/drivers/mtd/hyperbus/Makefile
> new file mode 100644
> index 000000000000..64e377d7f636
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_MTD_HYPERBUS) += core.o
> +obj-$(CONFIG_HBMC_AM654) += hbmc_am654.o

The above line clearly belongs to patch #5 as well...

> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
> new file mode 100644
> index 000000000000..d3d44aab7503
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/core.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +// Author: Vignesh R <[email protected]>
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/map.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/cfi.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/types.h>
> +
> +#define HB_CALIB_COUNT 25

Isn't this controller specific?

[...]
> +/* Calibrate HBMC by repeatedly reading "QRY" string from CFI space */
> +static int hb_calibrate(struct hb_device *hbdev)

s/int/bool/, perhaps?

[...]
> +int hb_register_device(struct hb_device *hbdev)
> +{
> + struct resource res;
> + struct device *dev;
> + struct device_node *np;
> + struct map_info *map;
> + struct hb_ops *ops;
> + int err;
> +
> + if (!hbdev || !hbdev->dev || !hbdev->np) {
> + pr_err("hyperbus: please fill all the necessary fields!\n");
> + return -EINVAL;
> + }
> +
> + np = hbdev->np;
> + if (!of_device_is_compatible(np, "cypress,hyperflash"))
> + return -ENODEV;
> +
> + hbdev->memtype = HYPERFLASH;
> +
> + if (of_address_to_resource(np, 0, &res))

Isn't the direct mapping property of the HF controller, not of HyperFlash
itself?

> + return -EINVAL;
> +
> + dev = hbdev->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;
> +
> + simple_map_init(map);

It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write
mappings in the separate memory resources.

[...]
> + if (hbdev->needs_calib) {
> + err = hb_calibrate(hbdev);
> + if (!err) {

Why call this variable 'err' when it indicates successful calibration?

> + dev_err(hbdev->dev, "Calibration failed\n");
> + return -ENODEV;
> + }
> + }
> +
> + hbdev->mtd = do_map_probe("cfi_probe", map);
> + if (!hbdev->mtd) {
> + dev_err(hbdev->dev, "probing failed\n");

"map probe", perhaps?

> + return -ENXIO;
> + }
> +
> + hbdev->mtd->dev.parent = hbdev->dev;
> + mtd_set_of_node(hbdev->mtd, np);
> +
> + err = mtd_device_register(hbdev->mtd, NULL, 0);
> + if (err) {
> + dev_err(hbdev->dev, "failed to register mtd device\n");
> + goto err_destroy;
> + }
> + hbdev->registered = true;
> +
> + return 0;
> +
> +err_destroy:

The label and the code below doesn't seem necessary. Just do it above
instead of *goto*.

> + map_destroy(hbdev->mtd);
> + return err;
> +}
[...]
> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
> new file mode 100644
> index 000000000000..0aa11458c424
> --- /dev/null
> +++ b/include/linux/mtd/hyperbus.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#ifndef __LINUX_MTD_HYPERBUS_H__
> +#define __LINUX_MTD_HYPERBUS_H__
> +
> +#include <linux/mtd/map.h>
> +
> +enum hb_memtype {

I'm for the full hyperbus_ prefixes, it's not that long and seems clearer.

[...]

MBR, Sergei

2019-02-24 14:08:14

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 5/5] mtd: hyperbus: Add driver for TI's Hyperbus memory controller

Hello!

On 19.02.2019 9:36, Vignesh R (by way of Boris Brezillon
<[email protected]>) wrote:

> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
> IP is pretty simple and provides direct memory mapped access to
> connected Flash devices.
>
> Add basic support for the IP without DMA. Second ChipSelect is not
> supported for now.
>
> Signed-off-by: Vignesh R <[email protected]>
> ---
> drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
> create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
>
> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
> new file mode 100644
> index 000000000000..1f0d2dc52f9f
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +// Author: Vignesh R <[email protected]>
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/types.h>
> +
> +struct am654_hbmc_priv {
> + struct hb_device hbdev;
> + void __iomem *regbase;
> +};
> +
> +static int am654_hbmc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct am654_hbmc_priv *priv;
> + struct resource *res;
> + int err;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res)) {

I don't think that function ever returns error ptrs. It should only
return NULL on error.

> + dev_err(&pdev->dev, "failed to get memory resource\n");
> + return -ENOENT;
> + }
> +
> + priv->regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->regbase)) {
> + dev_err(dev, "Cannot remap controller address.\n");

The above function already prints its error messages.

[...]
> +module_platform_driver(am654_hbmc_platform_driver);
> +
> +MODULE_DESCRIPTION("HBMC driver for AM654 SoC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:hbmc-am654");
> +MODULE_AUTHOR("Vignesh R <[email protected]>");

MBR, Sergei

2019-02-25 16:31:40

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 5/5] mtd: hyperbus: Add driver for TI's Hyperbus memory controller

Hello!

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
> IP is pretty simple and provides direct memory mapped access to
> connected Flash devices.

Are you sure you posted the _complete_ driver?

> Add basic support for the IP without DMA. Second ChipSelect is not
> supported for now.
>
> Signed-off-by: Vignesh R <[email protected]>
> ---
> drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
> create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
>
> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
> new file mode 100644
> index 000000000000..1f0d2dc52f9f
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
> @@ -0,0 +1,105 @@
[...]
> +static int am654_hbmc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct am654_hbmc_priv *priv;
> + struct resource *res;
> + int err;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res)) {
> + dev_err(&pdev->dev, "failed to get memory resource\n");
> + return -ENOENT;
> + }
> +
> + priv->regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->regbase)) {
> + dev_err(dev, "Cannot remap controller address.\n");
> + return PTR_ERR(priv->regbase);
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + err = pm_runtime_get_sync(&pdev->dev);

That's all nice but where's the code that accesses the actual registers?

> + if (err < 0) {
> + pm_runtime_put_noidle(&pdev->dev);

Calling "put" with previously failed "get" sees strange...

> + return err;
> + }
> +
> + priv->hbdev.needs_calib = true;
> + priv->hbdev.dev = &pdev->dev;
> + priv->hbdev.np = of_get_next_child(dev->of_node, NULL);
> + err = hb_register_device(&priv->hbdev);
> + if (err) {
> + dev_err(&pdev->dev, "failed to register controller\n");
> + goto err_destroy;
> + }
> +
> + return 0;
> +
> +err_destroy:
> + hb_unregister_device(&priv->hbdev);

Calling "unregister" with "register" previously failed also looks strange...

> + pm_runtime_put_sync(&pdev->dev);

Why the sync() version?

> + return err;
> +}
[...]

MBR, Sergei

2019-02-25 18:22:57

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

Hi Sergei,

On 24/02/19 1:49 AM, Sergei Shtylyov wrote:
> Hello!
>
> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:
>
>> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
>> interface between a host system master and one or more slave interfaces.
>> HyperBus is used to connect microprocessor, microcontroller, or ASIC
>> devices with random access NOR flash memory(called HyperFlash) or
>
> Need space before (.
>
>> self refresh DRAM(called HyperRAM).
>>
>> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
>> signal and either Single-ended clock(3.0V parts) or Differential clock
>> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
>> At bus level, it follows a separate protocol described in HyperBus
>> specification[1].
>>
>> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
>> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
>> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
>> operates at >166MHz frequencies.
>> HyperRAM provides direct random read/write access to flash memory
>> array.
>>
>> But, Hyperbus memory controllers seem to abstract implementation details
>> and expose a simple MMIO interface to access connected flash.
>>
>> Add support for registering HyperFlash devices with MTD framework. MTD
>> maps framework along with CFI chip support framework are used to support
>> communicate with flash.
>
> Communicating.
>
>> Framework is modelled along the lines of spi-nor framework. HyperBus
>> memory controller(HBMC) drivers call hb_register_device() to register a
>
> Again, space needed before (.
>
>> single HyperFlash device. HyperFlash core parses MMIO access
>> information from DT, sets up the map_info struct, probes CFI flash and
>> registers it with MTD framework.
>>
>> Some HBMC masters need calibration/training sequence[3] to be carried
>> out, in order for DLL inside the controller to lock, by reading a known
>> string/pattern. This is done by repeatedly reading CFI Query
>> Identification String. Calibration needs to be done before try to detect
>
> Trying.
>
>> flash as part of CFI flash probe.
>>
>> HyperRAM is not supported atm.
>
> ATM?
>
>> HyperBus specification can be found at[1]
>> HyperFlash datasheet can be found at[2]
>>
>> [1] https://www.cypress.com/file/213356/download
>> [2] https://www.cypress.com/file/213346/download
>> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
>> Table 12-5741. HyperFlash Access Sequence
>>
>> Signed-off-by: Vignesh R <[email protected]>
> [...]
>> diff --git a/drivers/mtd/hyperbus/Kconfig b/drivers/mtd/hyperbus/Kconfig
>> new file mode 100644
>> index 000000000000..02c38afc5c50
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/Kconfig
>> @@ -0,0 +1,23 @@
>> +menuconfig MTD_HYPERBUS
>> + tristate "Hyperbus support"
>> + select MTD_CFI
>> + select MTD_MAP_BANK_WIDTH_2
>> + select MTD_CFI_AMDSTD
>> + select MTD_COMPLEX_MAPPINGS
>> + help
>> + This is the framework for the Hyperbus which can be used by
>> + the Hyperbus Controller driver to commmunicate with
> ^^^
> Too many m's. :-)
>
>> + Hyperflash. See Cypress Hyperbus specification for more
>> + details
>> +
>> +
>> +if MTD_HYPERBUS
>> +
>> +config HBMC_AM654
>> + tristate "Hyperbus controller driver for AM65x SoC"
>> + help
>> + This is the driver for Hyperbus controller on TI's AM65x and
>> + other SoCs
>> +
>> +endif # MTD_HYPERBUS
>
> The above clearly belongs to patch #5.
>
>> +
>
> No empty lines at end of file, please...
>
>> diff --git a/drivers/mtd/hyperbus/Makefile b/drivers/mtd/hyperbus/Makefile
>> new file mode 100644
>> index 000000000000..64e377d7f636
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_MTD_HYPERBUS) += core.o
>> +obj-$(CONFIG_HBMC_AM654) += hbmc_am654.o
>
> The above line clearly belongs to patch #5 as well...

Right, will fix this and all the above comments.

>
>> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
>> new file mode 100644
>> index 000000000000..d3d44aab7503
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/core.c
>> @@ -0,0 +1,167 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +// Author: Vignesh R <[email protected]>
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mtd/hyperbus.h>
>> +#include <linux/mtd/map.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/cfi.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/types.h>
>> +
>> +#define HB_CALIB_COUNT 25
>
> Isn't this controller specific?
>
> [...]
>> +/* Calibrate HBMC by repeatedly reading "QRY" string from CFI space */
>> +static int hb_calibrate(struct hb_device *hbdev)
>
> s/int/bool/, perhaps?
>
> [...]
>> +int hb_register_device(struct hb_device *hbdev)
>> +{
>> + struct resource res;
>> + struct device *dev;
>> + struct device_node *np;
>> + struct map_info *map;
>> + struct hb_ops *ops;
>> + int err;
>> +
>> + if (!hbdev || !hbdev->dev || !hbdev->np) {
>> + pr_err("hyperbus: please fill all the necessary fields!\n");
>> + return -EINVAL;
>> + }
>> +
>> + np = hbdev->np;
>> + if (!of_device_is_compatible(np, "cypress,hyperflash"))
>> + return -ENODEV;
>> +
>> + hbdev->memtype = HYPERFLASH;
>> +
>> + if (of_address_to_resource(np, 0, &res))
>
> Isn't the direct mapping property of the HF controller, not of HyperFlash
> itself?
>

As I said in the cover letter, I could not find many examples of HF
controllers, but couple of them that I studied provide MMIO access to
flash. So, reg property of flash node would represent local address on
the HyperBus and controller node would set up ranges properly to provide
translation from CS to SoC address space.
For example see patch 4/5 where reg property would indicate CS and Size
of flash. This scheme is similar to whats described here [1]
HBMC controllers usually have different MMIO regions to access flashes
connected to different CS. So using ranges for address translation along
with flash node describing local address works pretty well.

My understanding is that this part of code will be common for most MMIO
based HB controllers and hence made part of core layer. But, if
controllers uses different IO space for read vs write, then this needs a
bit of thinking. In that case, mapping needs to be moved to controller
drivers.

[1]https://elinux.org/Device_Tree_Usage#Ranges_.28Address_Translation.29

>> + return -EINVAL;
>> +
>> + dev = hbdev->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;
>> +
>> + simple_map_init(map);
>
> It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write
> mappings in the separate memory resources.
>

Hmm, could you point me to public datasheet of the controller?
simple_map_init() provides default implementation for map operations
which is overridden, if hb_ops is populated.
I think, Renesas RPC-IF can populate custom hb_ops struct and use
appropriate MMIO base for read vs write, while still reusing the map
framework. Wouldnt that work?

> [...]
>> + if (hbdev->needs_calib) {
>> + err = hb_calibrate(hbdev);
>> + if (!err) {
>
> Why call this variable 'err' when it indicates successful calibration?
>
>> + dev_err(hbdev->dev, "Calibration failed\n");
>> + return -ENODEV;
>> + }
>> + }
>> +
>> + hbdev->mtd = do_map_probe("cfi_probe", map);
>> + if (!hbdev->mtd) {
>> + dev_err(hbdev->dev, "probing failed\n");
>
> "map probe", perhaps?
>
>> + return -ENXIO;
>> + }
>> +
>> + hbdev->mtd->dev.parent = hbdev->dev;
>> + mtd_set_of_node(hbdev->mtd, np);
>> +
>> + err = mtd_device_register(hbdev->mtd, NULL, 0);
>> + if (err) {
>> + dev_err(hbdev->dev, "failed to register mtd device\n");
>> + goto err_destroy;
>> + }
>> + hbdev->registered = true;
>> +
>> + return 0;
>> +
>> +err_destroy:
>
> The label and the code below doesn't seem necessary. Just do it above
> instead of *goto*.
>
>> + map_destroy(hbdev->mtd);
>> + return err;
>> +}
> [...]
>> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
>> new file mode 100644
>> index 000000000000..0aa11458c424
>> --- /dev/null
>> +++ b/include/linux/mtd/hyperbus.h
>> @@ -0,0 +1,73 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +#ifndef __LINUX_MTD_HYPERBUS_H__
>> +#define __LINUX_MTD_HYPERBUS_H__
>> +
>> +#include <linux/mtd/map.h>
>> +
>> +enum hb_memtype {
>
> I'm for the full hyperbus_ prefixes, it's not that long and seems clearer.
>
> [...]

Will address remaining comments on this patch in the next round. Thanks
for the review!

>
> MBR, Sergei
>

--
Regards
Vignesh

2019-02-25 18:27:13

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
> interface between a host system master and one or more slave interfaces.
> HyperBus is used to connect microprocessor, microcontroller, or ASIC
> devices with random access NOR flash memory(called HyperFlash) or
> self refresh DRAM(called HyperRAM).
>
> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
> signal and either Single-ended clock(3.0V parts) or Differential clock
> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
> At bus level, it follows a separate protocol described in HyperBus
> specification[1].
>
> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,

HyperBus, please be consistent.

> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus

Again...

> operates at >166MHz frequencies.
> HyperRAM provides direct random read/write access to flash memory
> array.
>
> But, Hyperbus memory controllers seem to abstract implementation details

And again.

> and expose a simple MMIO interface to access connected flash.
>
> Add support for registering HyperFlash devices with MTD framework. MTD
> maps framework along with CFI chip support framework are used to support
> communicate with flash.
>
> Framework is modelled along the lines of spi-nor framework. HyperBus
> memory controller(HBMC) drivers call hb_register_device() to register a
> single HyperFlash device. HyperFlash core parses MMIO access
> information from DT, sets up the map_info struct, probes CFI flash and
> registers it with MTD framework.
>
> Some HBMC masters need calibration/training sequence[3] to be carried
> out, in order for DLL inside the controller to lock, by reading a known
> string/pattern. This is done by repeatedly reading CFI Query
> Identification String. Calibration needs to be done before try to detect
> flash as part of CFI flash probe.
>
> HyperRAM is not supported atm.
>
> HyperBus specification can be found at[1]
> HyperFlash datasheet can be found at[2]
>
> [1] https://www.cypress.com/file/213356/download
> [2] https://www.cypress.com/file/213346/download
> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
> Table 12-5741. HyperFlash Access Sequence
>
> Signed-off-by: Vignesh R <[email protected]>
[...]
> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
> new file mode 100644
> index 000000000000..d3d44aab7503
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/core.c
> @@ -0,0 +1,167 @@
[...]
> +/* Calibrate HBMC by repeatedly reading "QRY" string from CFI space */
> +static int hb_calibrate(struct hb_device *hbdev)
> +{
> + struct map_info *map = &hbdev->map;
> + struct cfi_private cfi;
> + int count = HB_CALIB_COUNT;
> + int ret;
> +
> + cfi.interleave = 1;
> + cfi.device_type = CFI_DEVICETYPE_X16;
> + cfi_send_gen_cmd(0xF0, 0, 0, map, &cfi, cfi.device_type, NULL);
> + cfi_send_gen_cmd(0x98, 0x55, 0, map, &cfi, cfi.device_type, NULL);
> +
> + while (count--)
> + cfi_qry_present(map, 0, &cfi);

Why do all 25 reads if you can encounter valid QRY in less reads than 25?

> +
> + ret = cfi_qry_present(map, 0, &cfi);
> + cfi_qry_mode_off(0, map, &cfi);
> +
> + return ret;
> +}
[...]

MBR, Sergei

2019-02-25 19:34:02

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 02/25/2019 09:21 PM, Vignesh R wrote:

[...]

>>> HyperBus specification can be found at[1]
>>> HyperFlash datasheet can be found at[2]
>>>
>>> [1] https://www.cypress.com/file/213356/download
>>> [2] https://www.cypress.com/file/213346/download
>>> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
>>> Table 12-5741. HyperFlash Access Sequence
>>>
>>> Signed-off-by: Vignesh R <[email protected]>
[...]
>>> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
>>> new file mode 100644
>>> index 000000000000..d3d44aab7503
>>> --- /dev/null
>>> +++ b/drivers/mtd/hyperbus/core.c
[...]
>>> +int hb_register_device(struct hb_device *hbdev)
>>> +{
>>> + struct resource res;
>>> + struct device *dev;
>>> + struct device_node *np;
>>> + struct map_info *map;
>>> + struct hb_ops *ops;
>>> + int err;
>>> +
>>> + if (!hbdev || !hbdev->dev || !hbdev->np) {
>>> + pr_err("hyperbus: please fill all the necessary fields!\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + np = hbdev->np;
>>> + if (!of_device_is_compatible(np, "cypress,hyperflash"))
>>> + return -ENODEV;
>>> +
>>> + hbdev->memtype = HYPERFLASH;
>>> +
>>> + if (of_address_to_resource(np, 0, &res))
>>
>> Isn't the direct mapping property of the HF controller, not of HyperFlash
>> itself?
>>
>
> As I said in the cover letter, I could not find many examples of HF
> controllers, but couple of them that I studied provide MMIO access to
> flash. So, reg property of flash node would represent local address on
> the HyperBus and controller node would set up ranges properly to provide
> translation from CS to SoC address space.
> For example see patch 4/5 where reg property would indicate CS and Size
> of flash. This scheme is similar to whats described here [1]
> HBMC controllers usually have different MMIO regions to access flashes
> connected to different CS. So using ranges for address translation along
> with flash node describing local address works pretty well.
>
> My understanding is that this part of code will be common for most MMIO
> based HB controllers and hence made part of core layer. But, if
> controllers uses different IO space for read vs write, then this needs a
> bit of thinking. In that case, mapping needs to be moved to controller
> drivers.
>
> [1]https://elinux.org/Device_Tree_Usage#Ranges_.28Address_Translation.29
>
>>> + return -EINVAL;
>>> +
>>> + dev = hbdev->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;
>>> +
>>> + simple_map_init(map);
>>
>> It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write
>> mappings in the separate memory resources.
>>
>
> Hmm, could you point me to public datasheet of the controller?

See chapter 20 in [1]. Note that it's not the same SoC I'm developing for (R-Car
gen3 family, with NDA docs) but should be mostly the same RPC-IF core.

[1] https://www.renesas.com/us/en/doc/products/mpumcu/doc/rz/r01uh0746ej0200-rza2m.pdf?key=74862185b5e22ad09e648d21a35de615

> simple_map_init() provides default implementation for map operations
> which is overridden, if hb_ops is populated.
> I think, Renesas RPC-IF can populate custom hb_ops struct and use
> appropriate MMIO base for read vs write, while still reusing the map
> framework. Wouldnt that work?

It probably would...

[...]

MBR, Sergei

2019-02-26 10:18:06

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 5/5] mtd: hyperbus: Add driver for TI's Hyperbus memory controller

On 25/02/19 9:59 PM, Sergei Shtylyov wrote:
> Hello!
>
> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:
>
>> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
>> IP is pretty simple and provides direct memory mapped access to
>> connected Flash devices.
>
> Are you sure you posted the _complete_ driver?
>

Yes, it is... You can find controller doc here[1]. Default values in the
MCR/MTR registers are good enough to simple Hyperflash access.
More perf optimization and timing optimizations will come incrementally

[1] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf 12.3.3 Hyperbus Interface

>> Add basic support for the IP without DMA. Second ChipSelect is not
>> supported for now.
>>
>> Signed-off-by: Vignesh R <[email protected]>
>> ---
>> drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
>> 1 file changed, 105 insertions(+)
>> create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
>>
>> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
>> new file mode 100644
>> index 000000000000..1f0d2dc52f9f
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
>> @@ -0,0 +1,105 @@
> [...]
>> +static int am654_hbmc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct am654_hbmc_priv *priv;
>> + struct resource *res;
>> + int err;
>> +
>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, priv);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (IS_ERR(res)) {
>> + dev_err(&pdev->dev, "failed to get memory resource\n");
>> + return -ENOENT;
>> + }
>> +
>> + priv->regbase = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(priv->regbase)) {
>> + dev_err(dev, "Cannot remap controller address.\n");
>> + return PTR_ERR(priv->regbase);
>> + }
>> +
>> + pm_runtime_enable(&pdev->dev);
>> + err = pm_runtime_get_sync(&pdev->dev);
>
> That's all nice but where's the code that accesses the actual registers?

Interface and functional clk needs to be enabled even to access MMIO
space to read/write data to flash (done by the map framework). So driver
currently just enables everything during probe and disables on remove

>
>> + if (err < 0) {
>> + pm_runtime_put_noidle(&pdev->dev);
>
> Calling "put" with previously failed "get" sees strange...
>

Basically pm_runtime_get_sync() increments usage_count even in case of
failure and pm_runtime_put_noidle() puts it back. You can find many
examples of above piece of code in kernel.

>> + return err;
>> + }
>> +
>> + priv->hbdev.needs_calib = true;
>> + priv->hbdev.dev = &pdev->dev;
>> + priv->hbdev.np = of_get_next_child(dev->of_node, NULL);
>> + err = hb_register_device(&priv->hbdev);
>> + if (err) {
>> + dev_err(&pdev->dev, "failed to register controller\n");
>> + goto err_destroy;
>> + }
>> +
>> + return 0;
>> +
>> +err_destroy:
>> + hb_unregister_device(&priv->hbdev);
>
> Calling "unregister" with "register" previously failed also looks strange...
>

Agree, this is unneeded as hb_register_device() takes care of all
cleanups in err path.

>> + pm_runtime_put_sync(&pdev->dev);
>
> Why the sync() version?
>

Why not? Since the device is going away, I think its safer to ensure
device has definitely been put to idle state. I see its a common
practice in driver code.

>> + return err;
>> +}
> [...]
>
> MBR, Sergei
>

--
Regards
Vignesh

2019-02-26 10:26:57

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 26/02/19 1:03 AM, Sergei Shtylyov wrote:
> On 02/25/2019 09:21 PM, Vignesh R wrote:
>
> [...]
>
>>>> HyperBus specification can be found at[1]
>>>> HyperFlash datasheet can be found at[2]
>>>>
>>>> [1] https://www.cypress.com/file/213356/download
>>>> [2] https://www.cypress.com/file/213346/download
>>>> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
>>>> Table 12-5741. HyperFlash Access Sequence
>>>>
>>>> Signed-off-by: Vignesh R <[email protected]>
> [...]
>>>> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
>>>> new file mode 100644
>>>> index 000000000000..d3d44aab7503
>>>> --- /dev/null
>>>> +++ b/drivers/mtd/hyperbus/core.c
> [...]
>>>> +int hb_register_device(struct hb_device *hbdev)
>>>> +{
>>>> + struct resource res;
>>>> + struct device *dev;
>>>> + struct device_node *np;
>>>> + struct map_info *map;
>>>> + struct hb_ops *ops;
>>>> + int err;
>>>> +
>>>> + if (!hbdev || !hbdev->dev || !hbdev->np) {
>>>> + pr_err("hyperbus: please fill all the necessary fields!\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + np = hbdev->np;
>>>> + if (!of_device_is_compatible(np, "cypress,hyperflash"))
>>>> + return -ENODEV;
>>>> +
>>>> + hbdev->memtype = HYPERFLASH;
>>>> +
>>>> + if (of_address_to_resource(np, 0, &res))
>>>
>>> Isn't the direct mapping property of the HF controller, not of HyperFlash
>>> itself?
>>>
>>
>> As I said in the cover letter, I could not find many examples of HF
>> controllers, but couple of them that I studied provide MMIO access to
>> flash. So, reg property of flash node would represent local address on
>> the HyperBus and controller node would set up ranges properly to provide
>> translation from CS to SoC address space.
>> For example see patch 4/5 where reg property would indicate CS and Size
>> of flash. This scheme is similar to whats described here [1]
>> HBMC controllers usually have different MMIO regions to access flashes
>> connected to different CS. So using ranges for address translation along
>> with flash node describing local address works pretty well.
>>
>> My understanding is that this part of code will be common for most MMIO
>> based HB controllers and hence made part of core layer. But, if
>> controllers uses different IO space for read vs write, then this needs a
>> bit of thinking. In that case, mapping needs to be moved to controller
>> drivers.
>>
>> [1]https://elinux.org/Device_Tree_Usage#Ranges_.28Address_Translation.29
>>
>>>> + return -EINVAL;
>>>> +
>>>> + dev = hbdev->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;
>>>> +
>>>> + simple_map_init(map);
>>>
>>> It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write
>>> mappings in the separate memory resources.
>>>
>>
>> Hmm, could you point me to public datasheet of the controller?
>
> See chapter 20 in [1]. Note that it's not the same SoC I'm developing for (R-Car
> gen3 family, with NDA docs) but should be mostly the same RPC-IF core.
>
> [1] https://www.renesas.com/us/en/doc/products/mpumcu/doc/rz/r01uh0746ej0200-rza2m.pdf?key=74862185b5e22ad09e648d21a35de615

Thanks for the info!

>
>> simple_map_init() provides default implementation for map operations
>> which is overridden, if hb_ops is populated.
>> I think, Renesas RPC-IF can populate custom hb_ops struct and use
>> appropriate MMIO base for read vs write, while still reusing the map
>> framework. Wouldnt that work?
>
> It probably would...
>

Looking at above link, I see there are two HBMC controllers on Renesas
SoC. One is a dedicated HBMC controller(chapter 21) very similar to that
on TI SoC and a SPI Multi I/O Bus Controller (chapter 20) that also
supports Hyperbus protocol.

AFAICS, passing custom hb_ops is good enough to support both HW needs.
Let me know if something is missing. I would greatly appreciate if you
could test this series with your HW.

--
Regards
Vignesh

2019-02-26 11:01:38

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 24/02/19 1:49 AM, Sergei Shtylyov wrote:
> Hello!
>
> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:
>
>> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
>> interface between a host system master and one or more slave interfaces.
>> HyperBus is used to connect microprocessor, microcontroller, or ASIC
>> devices with random access NOR flash memory(called HyperFlash) or
>
> Need space before (.
>
>> self refresh DRAM(called HyperRAM).
>>
>> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
>> signal and either Single-ended clock(3.0V parts) or Differential clock
>> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
>> At bus level, it follows a separate protocol described in HyperBus
>> specification[1].
>>
>> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
>> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
>> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
>> operates at >166MHz frequencies.
>> HyperRAM provides direct random read/write access to flash memory
>> array.
>>
>> But, Hyperbus memory controllers seem to abstract implementation details
>> and expose a simple MMIO interface to access connected flash.
>>
>> Add support for registering HyperFlash devices with MTD framework. MTD
>> maps framework along with CFI chip support framework are used to support
>> communicate with flash.
>
> Communicating.
>
>> Framework is modelled along the lines of spi-nor framework. HyperBus
>> memory controller(HBMC) drivers call hb_register_device() to register a
>
> Again, space needed before (.
>
>> single HyperFlash device. HyperFlash core parses MMIO access
>> information from DT, sets up the map_info struct, probes CFI flash and
>> registers it with MTD framework.
>>
>> Some HBMC masters need calibration/training sequence[3] to be carried
>> out, in order for DLL inside the controller to lock, by reading a known
>> string/pattern. This is done by repeatedly reading CFI Query
>> Identification String. Calibration needs to be done before try to detect
>
> Trying.
>
>> flash as part of CFI flash probe.
>>
>> HyperRAM is not supported atm.
>
> ATM?
>
>> HyperBus specification can be found at[1]
>> HyperFlash datasheet can be found at[2]
>>
>> [1] https://www.cypress.com/file/213356/download
>> [2] https://www.cypress.com/file/213346/download
>> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
>> Table 12-5741. HyperFlash Access Sequence
>>
>> Signed-off-by: Vignesh R <[email protected]>
> [...]
>> diff --git a/drivers/mtd/hyperbus/Kconfig b/drivers/mtd/hyperbus/Kconfig
>> new file mode 100644
>> index 000000000000..02c38afc5c50
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/Kconfig
>> @@ -0,0 +1,23 @@
>> +menuconfig MTD_HYPERBUS
>> + tristate "Hyperbus support"
>> + select MTD_CFI
>> + select MTD_MAP_BANK_WIDTH_2
>> + select MTD_CFI_AMDSTD
>> + select MTD_COMPLEX_MAPPINGS
>> + help
>> + This is the framework for the Hyperbus which can be used by
>> + the Hyperbus Controller driver to commmunicate with
> ^^^
> Too many m's. :-)
>
>> + Hyperflash. See Cypress Hyperbus specification for more
>> + details
>> +
>> +
>> +if MTD_HYPERBUS
>> +
>> +config HBMC_AM654
>> + tristate "Hyperbus controller driver for AM65x SoC"
>> + help
>> + This is the driver for Hyperbus controller on TI's AM65x and
>> + other SoCs
>> +
>> +endif # MTD_HYPERBUS
>
> The above clearly belongs to patch #5.
>
>> +
>
> No empty lines at end of file, please...
>
>> diff --git a/drivers/mtd/hyperbus/Makefile b/drivers/mtd/hyperbus/Makefile
>> new file mode 100644
>> index 000000000000..64e377d7f636
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_MTD_HYPERBUS) += core.o
>> +obj-$(CONFIG_HBMC_AM654) += hbmc_am654.o
>
> The above line clearly belongs to patch #5 as well...
>
>> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
>> new file mode 100644
>> index 000000000000..d3d44aab7503
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/core.c
>> @@ -0,0 +1,167 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +// Author: Vignesh R <[email protected]>
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mtd/hyperbus.h>
>> +#include <linux/mtd/map.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/cfi.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/types.h>
>> +
>> +#define HB_CALIB_COUNT 25
>
> Isn't this controller specific?
>

I can convert this to be a field in hb_device struct that controller
driver can populate. But, I believe above count can still serve as
conservative default.

> [...]
>> +/* Calibrate HBMC by repeatedly reading "QRY" string from CFI space */
>> +static int hb_calibrate(struct hb_device *hbdev)
>
> s/int/bool/, perhaps?
>
> [...]
>> +int hb_register_device(struct hb_device *hbdev)
>> +{
>> + struct resource res;
>> + struct device *dev;
>> + struct device_node *np;
>> + struct map_info *map;
>> + struct hb_ops *ops;
>> + int err;
>> +
>> + if (!hbdev || !hbdev->dev || !hbdev->np) {
>> + pr_err("hyperbus: please fill all the necessary fields!\n");
>> + return -EINVAL;
>> + }
>> +
>> + np = hbdev->np;
>> + if (!of_device_is_compatible(np, "cypress,hyperflash"))
>> + return -ENODEV;
>> +
>> + hbdev->memtype = HYPERFLASH;
>> +
>> + if (of_address_to_resource(np, 0, &res))
>
> Isn't the direct mapping property of the HF controller, not of HyperFlash
> itself?
>
>> + return -EINVAL;
>> +
>> + dev = hbdev->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;
>> +
>> + simple_map_init(map);
>
> It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write
> mappings in the separate memory resources.
>
> [...]
>> + if (hbdev->needs_calib) {
>> + err = hb_calibrate(hbdev);
>> + if (!err) {
>
> Why call this variable 'err' when it indicates successful calibration?
>
>> + dev_err(hbdev->dev, "Calibration failed\n");
>> + return -ENODEV;
>> + }
>> + }
>> +
>> + hbdev->mtd = do_map_probe("cfi_probe", map);
>> + if (!hbdev->mtd) {
>> + dev_err(hbdev->dev, "probing failed\n");
>
> "map probe", perhaps?
>
>> + return -ENXIO;
>> + }
>> +
>> + hbdev->mtd->dev.parent = hbdev->dev;
>> + mtd_set_of_node(hbdev->mtd, np);
>> +
>> + err = mtd_device_register(hbdev->mtd, NULL, 0);
>> + if (err) {
>> + dev_err(hbdev->dev, "failed to register mtd device\n");
>> + goto err_destroy;
>> + }
>> + hbdev->registered = true;
>> +
>> + return 0;
>> +
>> +err_destroy:
>
> The label and the code below doesn't seem necessary. Just do it above
> instead of *goto*.
>
>> + map_destroy(hbdev->mtd);
>> + return err;
>> +}
> [...]
>> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
>> new file mode 100644
>> index 000000000000..0aa11458c424
>> --- /dev/null
>> +++ b/include/linux/mtd/hyperbus.h
>> @@ -0,0 +1,73 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +#ifndef __LINUX_MTD_HYPERBUS_H__
>> +#define __LINUX_MTD_HYPERBUS_H__
>> +
>> +#include <linux/mtd/map.h>
>> +
>> +enum hb_memtype {
>
> I'm for the full hyperbus_ prefixes, it's not that long and seems clearer.
>
> [...]
>
> MBR, Sergei
>

--
Regards
Vignesh

2019-02-26 11:10:29

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 02/26/2019 01:26 PM, Vignesh R wrote:

>> [...]

>>>>> HyperBus specification can be found at[1]
>>>>> HyperFlash datasheet can be found at[2]
>>>>>
>>>>> [1] https://www.cypress.com/file/213356/download
>>>>> [2] https://www.cypress.com/file/213346/download
>>>>> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
>>>>> Table 12-5741. HyperFlash Access Sequence
>>>>>
>>>>> Signed-off-by: Vignesh R <[email protected]>
>> [...]
>>>>> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c
>>>>> new file mode 100644
>>>>> index 000000000000..d3d44aab7503
>>>>> --- /dev/null
>>>>> +++ b/drivers/mtd/hyperbus/core.c
>> [...]
>>>>> + return -EINVAL;
>>>>> +
>>>>> + dev = hbdev->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;
>>>>> +
>>>>> + simple_map_init(map);
>>>>
>>>> It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write
>>>> mappings in the separate memory resources.
>>>>
>>>
>>> Hmm, could you point me to public datasheet of the controller?
>>
>> See chapter 20 in [1]. Note that it's not the same SoC I'm developing for (R-Car
>> gen3 family, with NDA docs) but should be mostly the same RPC-IF core.
>>
>> [1] https://www.renesas.com/us/en/doc/products/mpumcu/doc/rz/r01uh0746ej0200-rza2m.pdf?key=74862185b5e22ad09e648d21a35de615
>
> Thanks for the info!
>
>>> simple_map_init() provides default implementation for map operations
>>> which is overridden, if hb_ops is populated.
>>> I think, Renesas RPC-IF can populate custom hb_ops struct and use
>>> appropriate MMIO base for read vs write, while still reusing the map
>>> framework. Wouldnt that work?
>>
>> It probably would...
>
> Looking at above link, I see there are two HBMC controllers on Renesas
> SoC. One is a dedicated HBMC controller(chapter 21) very similar to that

We don't have this one in the R-Car gen3 -- I wasn't even aware of it. :-)
RZ/A2 is newer than gen3 SoCs.

> on TI SoC and a SPI Multi I/O Bus Controller (chapter 20) that also
> supports Hyperbus protocol.

That matches to the gen3 RPC-IF core.

> AFAICS, passing custom hb_ops is good enough to support both HW needs.
> Let me know if something is missing. I would greatly appreciate if you
> could test this series with your HW.

Yes, I have stated the conversion from the simple mapping driver.

MBR, Sergei

2019-02-26 18:17:25

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
> interface between a host system master and one or more slave interfaces.
> HyperBus is used to connect microprocessor, microcontroller, or ASIC
> devices with random access NOR flash memory(called HyperFlash) or
> self refresh DRAM(called HyperRAM).
>
> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
> signal and either Single-ended clock(3.0V parts) or Differential clock
> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
> At bus level, it follows a separate protocol described in HyperBus
> specification[1].
>
> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
> operates at >166MHz frequencies.
> HyperRAM provides direct random read/write access to flash memory
> array.
>
> But, Hyperbus memory controllers seem to abstract implementation details
> and expose a simple MMIO interface to access connected flash.
>
> Add support for registering HyperFlash devices with MTD framework. MTD
> maps framework along with CFI chip support framework are used to support
> communicate with flash.
>
> Framework is modelled along the lines of spi-nor framework. HyperBus
> memory controller(HBMC) drivers call hb_register_device() to register a
> single HyperFlash device. HyperFlash core parses MMIO access
> information from DT, sets up the map_info struct, probes CFI flash and
> registers it with MTD framework.
>
> Some HBMC masters need calibration/training sequence[3] to be carried
> out, in order for DLL inside the controller to lock, by reading a known
> string/pattern. This is done by repeatedly reading CFI Query
> Identification String. Calibration needs to be done before try to detect
> flash as part of CFI flash probe.
>
> HyperRAM is not supported atm.
>
> HyperBus specification can be found at[1]
> HyperFlash datasheet can be found at[2]
>
> [1] https://www.cypress.com/file/213356/download
> [2] https://www.cypress.com/file/213346/download
> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
> Table 12-5741. HyperFlash Access Sequence
>
> Signed-off-by: Vignesh R <[email protected]>
[...]
> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
> new file mode 100644
> index 000000000000..0aa11458c424
> --- /dev/null
> +++ b/include/linux/mtd/hyperbus.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#ifndef __LINUX_MTD_HYPERBUS_H__
> +#define __LINUX_MTD_HYPERBUS_H__
> +
> +#include <linux/mtd/map.h>
> +
> +enum hb_memtype {
> + HYPERFLASH,
> + HYPERRAM,
> +};
> +
> +/**
> + * struct hb_device - struct representing Hyperbus slave device
> + * @map: map_info struct for accessing MMIO Hyperbus flash memory
> + * @dev: device pointer of Hyperbus Controller

I think we need a separate structure for the HyperBus controller, not just
for the slave devices...

> + * @np: pointer to Hyperbus slave device node
> + * @mtd: pointer to MTD struct
> + * @ops: pointer to custom Hyperbus ops
> + * @memtype: type of memory device: Hyperflash or HyperRAM
> + * @needs_calib: flag to indicate whether calibration sequence is needed
> + * @registered: flag to indicate whether device is registered with MTD core
> + */
> +
> +struct hb_device {
> + struct map_info map;
> + struct device *dev;
> + struct device_node *np;
> + struct mtd_info *mtd;
> + struct hb_ops *ops;
> + enum hb_memtype memtype;
> + bool needs_calib;
> + bool registered;
> +};
> +
> +/**
> + * struct hb_ops - struct representing custom Hyperbus operations
> + * @read16: read 16 bit of data, usually from register/ID-CFI space
> + * @write16: write 16 bit of data, usually to register/ID-CFI space
> + * copy_from: copy data from flash memory
> + * copy_to: copy data to flash_memory
> + */
> +
> +struct hb_ops {
> + u16 (*read16)(struct hb_device *hbdev, unsigned long addr);
> + void (*write16)(struct hb_device *hbdev, unsigned long addr, u16 val);
> +
> + void (*copy_from)(struct hb_device *hbdev, void *to,
> + unsigned long from, ssize_t len);
> + void (*copy_to)(struct hb_device *dev, unsigned long to,
> + const void *from, ssize_t len);

... else these methods won't fly if you need to "massage" the controller
registers inside them...

> +};
[...]

MBR, Sergei

2019-02-26 18:30:14

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [RFC PATCH 5/5] mtd: hyperbus: Add driver for TI's Hyperbus memory controller

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:

> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
> IP is pretty simple and provides direct memory mapped access to
> connected Flash devices.
>
> Add basic support for the IP without DMA. Second ChipSelect is not
> supported for now.
>
> Signed-off-by: Vignesh R <[email protected]>
> ---
> drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
> create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
>
> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
> new file mode 100644
> index 000000000000..1f0d2dc52f9f
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +// Author: Vignesh R <[email protected]>
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/types.h>
> +
> +struct am654_hbmc_priv {
> + struct hb_device hbdev;
> + void __iomem *regbase;

Isn't regbase a controller's data, not a slave device's?

[...]

MBR, Sergei

2019-02-27 09:54:27

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 26/02/19 11:46 PM, Sergei Shtylyov wrote:
> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:
>
>> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
>> interface between a host system master and one or more slave interfaces.
>> HyperBus is used to connect microprocessor, microcontroller, or ASIC
>> devices with random access NOR flash memory(called HyperFlash) or
>> self refresh DRAM(called HyperRAM).
>>
>> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
>> signal and either Single-ended clock(3.0V parts) or Differential clock
>> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
>> At bus level, it follows a separate protocol described in HyperBus
>> specification[1].
>>
>> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
>> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
>> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
>> operates at >166MHz frequencies.
>> HyperRAM provides direct random read/write access to flash memory
>> array.
>>
>> But, Hyperbus memory controllers seem to abstract implementation details
>> and expose a simple MMIO interface to access connected flash.
>>
>> Add support for registering HyperFlash devices with MTD framework. MTD
>> maps framework along with CFI chip support framework are used to support
>> communicate with flash.
>>
>> Framework is modelled along the lines of spi-nor framework. HyperBus
>> memory controller(HBMC) drivers call hb_register_device() to register a
>> single HyperFlash device. HyperFlash core parses MMIO access
>> information from DT, sets up the map_info struct, probes CFI flash and
>> registers it with MTD framework.
>>
>> Some HBMC masters need calibration/training sequence[3] to be carried
>> out, in order for DLL inside the controller to lock, by reading a known
>> string/pattern. This is done by repeatedly reading CFI Query
>> Identification String. Calibration needs to be done before try to detect
>> flash as part of CFI flash probe.
>>
>> HyperRAM is not supported atm.
>>
>> HyperBus specification can be found at[1]
>> HyperFlash datasheet can be found at[2]
>>
>> [1] https://www.cypress.com/file/213356/download
>> [2] https://www.cypress.com/file/213346/download
>> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
>> Table 12-5741. HyperFlash Access Sequence
>>
>> Signed-off-by: Vignesh R <[email protected]>
> [...]
>> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
>> new file mode 100644
>> index 000000000000..0aa11458c424
>> --- /dev/null
>> +++ b/include/linux/mtd/hyperbus.h
>> @@ -0,0 +1,73 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +#ifndef __LINUX_MTD_HYPERBUS_H__
>> +#define __LINUX_MTD_HYPERBUS_H__
>> +
>> +#include <linux/mtd/map.h>
>> +
>> +enum hb_memtype {
>> + HYPERFLASH,
>> + HYPERRAM,
>> +};
>> +
>> +/**
>> + * struct hb_device - struct representing Hyperbus slave device
>> + * @map: map_info struct for accessing MMIO Hyperbus flash memory
>> + * @dev: device pointer of Hyperbus Controller
>
> I think we need a separate structure for the HyperBus controller, not just
> for the slave devices...
>
>> + * @np: pointer to Hyperbus slave device node
>> + * @mtd: pointer to MTD struct
>> + * @ops: pointer to custom Hyperbus ops
>> + * @memtype: type of memory device: Hyperflash or HyperRAM
>> + * @needs_calib: flag to indicate whether calibration sequence is needed
>> + * @registered: flag to indicate whether device is registered with MTD core
>> + */
>> +
>> +struct hb_device {
>> + struct map_info map;
>> + struct device *dev;
>> + struct device_node *np;
>> + struct mtd_info *mtd;
>> + struct hb_ops *ops;
>> + enum hb_memtype memtype;
>> + bool needs_calib;
>> + bool registered;
>> +};
>> +
>> +/**
>> + * struct hb_ops - struct representing custom Hyperbus operations
>> + * @read16: read 16 bit of data, usually from register/ID-CFI space
>> + * @write16: write 16 bit of data, usually to register/ID-CFI space
>> + * copy_from: copy data from flash memory
>> + * copy_to: copy data to flash_memory
>> + */
>> +
>> +struct hb_ops {
>> + u16 (*read16)(struct hb_device *hbdev, unsigned long addr);
>> + void (*write16)(struct hb_device *hbdev, unsigned long addr, u16 val);
>> +
>> + void (*copy_from)(struct hb_device *hbdev, void *to,
>> + unsigned long from, ssize_t len);
>> + void (*copy_to)(struct hb_device *dev, unsigned long to,
>> + const void *from, ssize_t len);
>
> ... else these methods won't fly if you need to "massage" the controller
> registers inside them...
>

If accessing controller register is the only need, wouldn't a private
data pointer within struct hb_device be sufficient to hold pointer to
controller specific struct?

struct hb_device {
...
void *priv; /* points to controller's private data */
};

Or do you see a need for separate structure for the HyperBus controller?

>> +};
> [...]
>
> MBR, Sergei
>

--
Regards
Vignesh

2019-02-27 10:00:41

by Boris Brezillon

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On Wed, 27 Feb 2019 15:22:19 +0530
Vignesh Raghavendra <[email protected]> wrote:

> On 26/02/19 11:46 PM, Sergei Shtylyov wrote:
> > On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:
> >
> >> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus
> >> interface between a host system master and one or more slave interfaces.
> >> HyperBus is used to connect microprocessor, microcontroller, or ASIC
> >> devices with random access NOR flash memory(called HyperFlash) or
> >> self refresh DRAM(called HyperRAM).
> >>
> >> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS)
> >> signal and either Single-ended clock(3.0V parts) or Differential clock
> >> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves.
> >> At bus level, it follows a separate protocol described in HyperBus
> >> specification[1].
> >>
> >> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar
> >> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus,
> >> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus
> >> operates at >166MHz frequencies.
> >> HyperRAM provides direct random read/write access to flash memory
> >> array.
> >>
> >> But, Hyperbus memory controllers seem to abstract implementation details
> >> and expose a simple MMIO interface to access connected flash.
> >>
> >> Add support for registering HyperFlash devices with MTD framework. MTD
> >> maps framework along with CFI chip support framework are used to support
> >> communicate with flash.
> >>
> >> Framework is modelled along the lines of spi-nor framework. HyperBus
> >> memory controller(HBMC) drivers call hb_register_device() to register a
> >> single HyperFlash device. HyperFlash core parses MMIO access
> >> information from DT, sets up the map_info struct, probes CFI flash and
> >> registers it with MTD framework.
> >>
> >> Some HBMC masters need calibration/training sequence[3] to be carried
> >> out, in order for DLL inside the controller to lock, by reading a known
> >> string/pattern. This is done by repeatedly reading CFI Query
> >> Identification String. Calibration needs to be done before try to detect
> >> flash as part of CFI flash probe.
> >>
> >> HyperRAM is not supported atm.
> >>
> >> HyperBus specification can be found at[1]
> >> HyperFlash datasheet can be found at[2]
> >>
> >> [1] https://www.cypress.com/file/213356/download
> >> [2] https://www.cypress.com/file/213346/download
> >> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf
> >> Table 12-5741. HyperFlash Access Sequence
> >>
> >> Signed-off-by: Vignesh R <[email protected]>
> > [...]
> >> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
> >> new file mode 100644
> >> index 000000000000..0aa11458c424
> >> --- /dev/null
> >> +++ b/include/linux/mtd/hyperbus.h
> >> @@ -0,0 +1,73 @@
> >> +/* SPDX-License-Identifier: GPL-2.0
> >> + *
> >> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> >> + */
> >> +
> >> +#ifndef __LINUX_MTD_HYPERBUS_H__
> >> +#define __LINUX_MTD_HYPERBUS_H__
> >> +
> >> +#include <linux/mtd/map.h>
> >> +
> >> +enum hb_memtype {
> >> + HYPERFLASH,
> >> + HYPERRAM,
> >> +};
> >> +
> >> +/**
> >> + * struct hb_device - struct representing Hyperbus slave device
> >> + * @map: map_info struct for accessing MMIO Hyperbus flash memory
> >> + * @dev: device pointer of Hyperbus Controller
> >
> > I think we need a separate structure for the HyperBus controller, not just
> > for the slave devices...
> >
> >> + * @np: pointer to Hyperbus slave device node
> >> + * @mtd: pointer to MTD struct
> >> + * @ops: pointer to custom Hyperbus ops
> >> + * @memtype: type of memory device: Hyperflash or HyperRAM
> >> + * @needs_calib: flag to indicate whether calibration sequence is needed
> >> + * @registered: flag to indicate whether device is registered with MTD core
> >> + */
> >> +
> >> +struct hb_device {
> >> + struct map_info map;
> >> + struct device *dev;
> >> + struct device_node *np;
> >> + struct mtd_info *mtd;
> >> + struct hb_ops *ops;
> >> + enum hb_memtype memtype;
> >> + bool needs_calib;
> >> + bool registered;
> >> +};
> >> +
> >> +/**
> >> + * struct hb_ops - struct representing custom Hyperbus operations
> >> + * @read16: read 16 bit of data, usually from register/ID-CFI space
> >> + * @write16: write 16 bit of data, usually to register/ID-CFI space
> >> + * copy_from: copy data from flash memory
> >> + * copy_to: copy data to flash_memory
> >> + */
> >> +
> >> +struct hb_ops {
> >> + u16 (*read16)(struct hb_device *hbdev, unsigned long addr);
> >> + void (*write16)(struct hb_device *hbdev, unsigned long addr, u16 val);
> >> +
> >> + void (*copy_from)(struct hb_device *hbdev, void *to,
> >> + unsigned long from, ssize_t len);
> >> + void (*copy_to)(struct hb_device *dev, unsigned long to,
> >> + const void *from, ssize_t len);
> >
> > ... else these methods won't fly if you need to "massage" the controller
> > registers inside them...
> >
>
> If accessing controller register is the only need, wouldn't a private
> data pointer within struct hb_device be sufficient to hold pointer to
> controller specific struct?
>
> struct hb_device {
> ...
> void *priv; /* points to controller's private data */
> };
>
>
> Or do you see a need for separate structure for the HyperBus controller?

Sorry to chime in. Just want to share my experience here: properly
splitting the controller/device representation is always a good thing.
When it's not done from the beginning and people start to add their own
controller drivers as if it was a flash device driver it becomes messy
pretty quickly and people add hacks to support that (look at the raw
NAND framework if you need a proof). So, I'd recommend having this
separation now, even if the onle controllers we support have a 1:1
relationship between HB controller and HB device.

>
>
> >> +};
> > [...]
> >
> > MBR, Sergei
> >
>

2019-02-27 10:12:28

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices

On 27/02/19 3:29 PM, Boris Brezillon wrote:
> On Wed, 27 Feb 2019 15:22:19 +0530
> Vignesh Raghavendra <[email protected]> wrote:
>
>> On 26/02/19 11:46 PM, Sergei Shtylyov wrote:
>>> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <[email protected]>) wrote:
>>>
>
[...]
>>>> + */
>>>> +
>>>> +struct hb_device {
>>>> + struct map_info map;
>>>> + struct device *dev;
>>>> + struct device_node *np;
>>>> + struct mtd_info *mtd;
>>>> + struct hb_ops *ops;
>>>> + enum hb_memtype memtype;
>>>> + bool needs_calib;
>>>> + bool registered;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct hb_ops - struct representing custom Hyperbus operations
>>>> + * @read16: read 16 bit of data, usually from register/ID-CFI space
>>>> + * @write16: write 16 bit of data, usually to register/ID-CFI space
>>>> + * copy_from: copy data from flash memory
>>>> + * copy_to: copy data to flash_memory
>>>> + */
>>>> +
>>>> +struct hb_ops {
>>>> + u16 (*read16)(struct hb_device *hbdev, unsigned long addr);
>>>> + void (*write16)(struct hb_device *hbdev, unsigned long addr, u16 val);
>>>> +
>>>> + void (*copy_from)(struct hb_device *hbdev, void *to,
>>>> + unsigned long from, ssize_t len);
>>>> + void (*copy_to)(struct hb_device *dev, unsigned long to,
>>>> + const void *from, ssize_t len);
>>>
>>> ... else these methods won't fly if you need to "massage" the controller
>>> registers inside them...
>>>
>>
>> If accessing controller register is the only need, wouldn't a private
>> data pointer within struct hb_device be sufficient to hold pointer to
>> controller specific struct?
>>
>> struct hb_device {
>> ...
>> void *priv; /* points to controller's private data */
>> };
>>
>>
>> Or do you see a need for separate structure for the HyperBus controller?
>
> Sorry to chime in. Just want to share my experience here: properly
> splitting the controller/device representation is always a good thing.
> When it's not done from the beginning and people start to add their own
> controller drivers as if it was a flash device driver it becomes messy
> pretty quickly and people add hacks to support that (look at the raw
> NAND framework if you need a proof). So, I'd recommend having this
> separation now, even if the onle controllers we support have a 1:1
> relationship between HB controller and HB device.
>
>>

Alright, will separate controller and slave representation. Thanks for
the feedback!

--
Regards
Vignesh