Messages in this thread | | | Date | Tue, 4 Sep 2018 16:58:42 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH 3/7] spi: spi-mem: Add a driver for NXP FlexSPI controller |
| |
Hi Yogesh,
On Fri, 31 Aug 2018 16:00:00 +0530 Yogesh Gaur <yogeshnarayan.gaur@nxp.com> wrote:
> - Add a driver for NXP FlexSPI host controller > > (0) What is the FlexSPI controller? > FlexSPI is a flexsible SPI host controller which supports two SPI > channels and up to 4 external devices. > Each channel supports Single/Dual/Quad/Octal mode data > transfer (1/2/4/8 bidirectional data lines) > i.e. FlexSPI acts as an interface to external devices, > maximum 4, each with up to 8 bidirectional data lines. > > It uses new SPI memory interface of the SPI framework to issue flash > memory operations to up to four connected flash chips (2 buses with > 2 CS each). > Chipselect for each flash can be selected as per address assigned in > controller specific registers. > > FlexSPI controller is similar to the existing Freescale/NXP QuadSPI > controller with advanced features.
Yep, I had a quick look at the code and they really look similar. Why not extending the existing driver instead of creating a new one from scratch?
> > (1) The FlexSPI controller is driven by the LUT(Look-up Table) > registers. > The LUT registers are a look-up-table for sequences of instructions. > A valid sequence consists of four LUT registers. > Maximum 32 LUT sequences can be programmed simultaneously. > > (2) The definition of the LUT register shows below: > --------------------------------------------------- > | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 | > --------------------------------------------------- > > There are several types of INSTRx, such as: > CMD : the SPI NOR command. > ADDR : the address for the SPI NOR command. > DUMMY : the dummy cycles needed by the SPI NOR command. > .... > > There are several types of PADx, such as: > PAD1 : use a singe I/O line. > PAD2 : use two I/O lines. > PAD4 : use quad I/O lines. > PAD8 : use octal I/O lines. > .... > > (3) LUTs are being created at run-time based on the commands passed > from the spi-mem framework. Thus, using single LUT index. > > (4) Software triggered Flash read/write access by IP Bus. > > (5) Memory mapped read access by AHB Bus.
Do we really want to have this level of details in the commit message? I mean, this is something you can document in the driver, but not here.
BTW, you might want to have a look at [1]. I think we can get rid of the ->size field you're adding if this driver implements the dirmap hooks.
> > (6) Tested this driver with the mtd_debug and JFFS2 filesystem utility > on NXP LX2160ARDB and LX2160AQDS targets. > LX2160ARDB is having two NOR slave device connected on single bus A > i.e. A0 and A1 (CS0 and CS1). > LX2160AQDS is having two NOR slave device connected on separate buses > one flash on A0 and second on B1 i.e. (CS0 and CS3). > Verified this driver on following SPI NOR flashes: > Micron, mt35xu512ab, [Read - 1 bit mode] > Cypress, s25fl512s, [Read - 1/2/4 bit mode]
Ok, that's good to have in the commit message.
> > - Add config option entry in 'spi-nor/Kconfig' for FlexSPI driver.
But this one is useless. If you add a new driver, you have no other choice but to add a new entry in the Kconfig file.
> > - Add entry in the 'spi-nor/Makefile'. >
Ditto.
Before you re-send a new version, I'd like to understand why you think you need to create a new driver, and I want you to try to implement the dirmap hook and check if you can get rid of the ->size field when doing that.
I also seem one extra benefit to having a single driver for both FlexSPI and QuadSPI IPs: you'll help Frieder debug the last remaining problems you reported on the new QuadSPI driver :-P.
Thanks,
Boris
| |