| Message ID | 20190219063607.29949-4-vigneshr@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MTD: Add Initial Hyperbus support | expand |
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
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 >
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
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
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.
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 >
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
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
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 >
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 > > >
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!
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__ */
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