[RFC,3/5] mtd: Add support for Hyperbus memory devices
diff mbox series

Message ID 20190219063607.29949-4-vigneshr@ti.com
State Superseded
Headers show
Series
  • MTD: Add Initial Hyperbus support
Related show

Commit Message

Vignesh Raghavendra Feb. 19, 2019, 6:36 a.m. UTC
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 <vigneshr@ti.com>
---
 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

Comments

Sergei Shtylyov Feb. 23, 2019, 8:19 p.m. UTC | #1
Hello!

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>
[...]
> 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 <vigneshr@ti.com>
> +
> +#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
Vignesh Raghavendra Feb. 25, 2019, 6:21 p.m. UTC | #2
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 <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>
> [...]
>> 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 <vigneshr@ti.com>
>> +
>> +#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
>
Sergei Shtylyov Feb. 25, 2019, 6:26 p.m. UTC | #3
On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>
[...]
> 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
Sergei Shtylyov Feb. 25, 2019, 7:33 p.m. UTC | #4
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 <vigneshr@ti.com>
[...]
>>> 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
Vignesh Raghavendra Feb. 26, 2019, 10:26 a.m. UTC | #5
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 <vigneshr@ti.com>
> [...]
>>>> 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.
Vignesh Raghavendra Feb. 26, 2019, 11 a.m. UTC | #6
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 <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>
> [...]
>> 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 <vigneshr@ti.com>
>> +
>> +#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
>
Sergei Shtylyov Feb. 26, 2019, 11:08 a.m. UTC | #7
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 <vigneshr@ti.com>
>> [...]
>>>>> 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
Sergei Shtylyov Feb. 26, 2019, 6:16 p.m. UTC | #8
On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>
[...]
> 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
Vignesh Raghavendra Feb. 27, 2019, 9:52 a.m. UTC | #9
On 26/02/19 11:46 PM, Sergei Shtylyov wrote:
> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>
> [...]
>> 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
>
Boris Brezillon Feb. 27, 2019, 9:59 a.m. UTC | #10
On Wed, 27 Feb 2019 15:22:19 +0530
Vignesh Raghavendra <vigneshr@ti.com> 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 <bbrezillon@kernel.org>) 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 <vigneshr@ti.com>  
> > [...]  
> >> 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
> >   
>
Vignesh Raghavendra Feb. 27, 2019, 10:12 a.m. UTC | #11
On 27/02/19 3:29 PM, Boris Brezillon wrote:
> On Wed, 27 Feb 2019 15:22:19 +0530
> Vignesh Raghavendra <vigneshr@ti.com> 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 <bbrezillon@kernel.org>) 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!

Patch
diff mbox series

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 <vigneshr@ti.com>
+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:	linuxppc-dev@lists.ozlabs.org
 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 <vigneshr@ti.com>
+
+#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__ */