Message-ID: <f47d4d57-afb2-3b39-fae9-3ed740a2b8a6@ti.com>
Date:   Tue, 11 Jun 2019 17:27:25 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Boris Brezillon <bbrezillon@...nel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Rob Herring <robh+dt@...nel.org>
CC:     <linux-mtd@...ts.infradead.org>,
        Tudor Ambarus <Tudor.Ambarus@...rochip.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Joakim Tjernlund <Joakim.Tjernlund@...inera.com>,
        <devicetree@...r.kernel.org>, Mason Yang <masonccyang@...c.com.tw>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 3/5] mtd: Add support for HyperBus memory devices



On 10/06/19 11:27 PM, Sergei Shtylyov wrote:
> On 06/09/2019 01:32 PM, Vignesh Raghavendra 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 clock cycle. 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
>> communicating with flash.
>>
>> Framework is modelled along the lines of spi-nor framework. HyperBus
>> memory controller (HBMC) drivers calls hyperbus_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 trying to detect
>> flash as part of CFI flash probe.
>>
>> HyperRAM is not supported at the moment.
>>
>> 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 Raghavendra <vigneshr@...com>
> [...]
>> diff --git a/drivers/mtd/hyperbus/hyperbus-core.c b/drivers/mtd/hyperbus/hyperbus-core.c
>> new file mode 100644
>> index 000000000000..df1f75e10b1a
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/hyperbus-core.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +// Author: Vignesh Raghavendra <vigneshr@...com>
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.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 HYPERBUS_CALIB_COUNT 25
> 
>    Mhm, I think I've already protested about this being #define'd here...
> 

I thought you had agreed that default optional calibration routine can
be part of core code and thus this #define.

Anyways, what is your preference here? Drop the constant and use a local
variable in hyperbus_calibrate()?
Or are you suggesting to move hyperbus_calibrate() TI's specific driver?


> [...]
>> +int hyperbus_register_device(struct hyperbus_device *hbdev)
>> +{
>> +	const struct hyperbus_ops *ops;
>> +	struct hyperbus_ctlr *ctlr;
>> +	struct device_node *np;
>> +	struct map_info *map;
>> +	struct resource res;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	if (!hbdev || !hbdev->np || !hbdev->ctlr || !hbdev->ctlr->dev) {
>> +		pr_err("hyperbus: please fill all the necessary fields!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	np = hbdev->np;
>> +	ctlr = hbdev->ctlr;
>> +	if (!of_device_is_compatible(np, "cypress,hyperflash"))
>> +		return -ENODEV;
>> +
>> +	hbdev->memtype = HYPERFLASH;
>> +
>> +	if (of_address_to_resource(np, 0, &res))
>> +		return -EINVAL;
> 
>    Why not just propagate the error upstream (yeah, I've noticed that
> it only can be -EINVAL)?
> 

Ok.

> [...]
>> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h
>> new file mode 100644
>> index 000000000000..ee2eefd822c9
>> --- /dev/null
>> +++ b/include/linux/mtd/hyperbus.h
>> @@ -0,0 +1,91 @@
>> +/* 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 hyperbus_memtype {
>> +	HYPERFLASH,
>> +	HYPERRAM,
>> +};
>> +
>> +/**
>> + * struct hyperbus_device - struct representing HyperBus slave device
>> + * @map: map_info struct for accessing MMIO HyperBus flash memory
>> + * @np:	pointer to HyperBus slave device node
>           ^
>    Space needed here, not tab.
> 

Ok

>> + * @mtd: pointer to MTD struct
>> + * @ctlr: pointer to HyperBus controller struct
>> + * @memtype: type of memory device: HyperFlash or HyperRAM
>> + * @registered: flag to indicate whether device is registered with MTD core
>> + */
>> +
>> +struct hyperbus_device {
>> +	struct map_info map;
>> +	struct device_node *np;
>> +	struct mtd_info *mtd;
>> +	struct hyperbus_ctlr *ctlr;
>> +	enum hyperbus_memtype memtype;
>> +	bool registered;
>> +};
>> +
>> +/**
>> + * struct hyperbus_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
> 
>    Usually? How to differ the register/memory transfers if both are possible?
> 

CFI + map framework does not provide a way to differentiate b/w reg
access vs memory access. read16()/write16() is used to either access
registers or for sending various cmds like lock/unlock etc or for
programming a single word.
For regular read/writes copy_from() and copy_to() are used.

Looking at HyperBus protocol, controllers would not need to
differentiate b/w registers vs memory transfers for HyperFlash devices.
So, I think I can drop read16/write16 and redirect these calls to
copy_from()/copy_to()


I mainly added these functions keeping HyperRAM in mind. Idea was
drivers would look at hyperbus_device->memtype and set to register
access mode for HyperRAM in case of write16()/read16(). Looks like the
interface is not intuitive enough
So, will drop these and add it back when adding HyperRAM support.

Does that work for your HW as well?

>> + * @copy_from: copy data from flash memory
>> + * @copy_to: copy data to flash memory
>> + * @calibrate: calibrate HyperBus controller
>> + */
>> +
>> +struct hyperbus_ops {
>> +	u16 (*read16)(struct hyperbus_device *hbdev, unsigned long addr);
>> +	void (*write16)(struct hyperbus_device *hbdev,
>> +			unsigned long addr, u16 val);
>> +	void (*copy_from)(struct hyperbus_device *hbdev, void *to,
>> +			  unsigned long from, ssize_t len);
>> +	void (*copy_to)(struct hyperbus_device *dev, unsigned long to,
>> +			const void *from, ssize_t len);
>> +	int (*calibrate)(struct hyperbus_device *dev);
>> +};
>> +
>> +/**
>> + * struct hyperbus_ctlr - struct representing HyperBus controller
>> + * @calibrated: flag to indicate ctlr calibration sequence is complete
>> + * @ops: HyperBus controller ops
> 
>    What about @dev?
> 

Will add.

>> + */
>> +struct hyperbus_ctlr {
>> +	struct device *dev;
>> +	bool calibrated;
>> +
>> +	const struct hyperbus_ops *ops;
>> +};
> [...]
> 
> MBR, Sergei
> 

-- 
Regards
Vignesh

Powered by blists - more mailing lists