Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752107AbbHTJCH (ORCPT ); Thu, 20 Aug 2015 05:02:07 -0400 Received: from mail-out.m-online.net ([212.18.0.10]:44676 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751690AbbHTJCC (ORCPT ); Thu, 20 Aug 2015 05:02:02 -0400 X-Auth-Info: ZcwY1h/iai8Qo0wrxuA3z6Pf4m0PtZqq5xX23sko3Pg= From: Marek Vasut To: Nga Chi Subject: Re: [PATCH] [PATCH v5] mtd:spi-nor: Add Altera Quad SPI Driver Date: Thu, 20 Aug 2015 10:52:47 +0200 User-Agent: KMail/1.13.7 (Linux/3.14-2-amd64; KDE/4.13.1; x86_64; ; ) Cc: "linux-mtd@lists.infradead.org" , Viet Nga Dao , Brian Norris , David Woodhouse , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <1440053705-3836-1-git-send-email-vndao@altera.com> <201508201003.38179.marex@denx.de> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201508201052.47502.marex@denx.de> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9255 Lines: 208 On Thursday, August 20, 2015 at 10:13:30 AM, Nga Chi wrote: > On Thu, Aug 20, 2015 at 4:03 PM, Marek Vasut wrote: > > On Thursday, August 20, 2015 at 08:55:05 AM, vndao@altera.com wrote: > >> From: VIET NGA DAO > >> > >> Altera Quad SPI Controller is a soft IP which enables access to > >> Altera EPCS and EPCQ flash chips. This patch adds driver > >> for these devices. > >> > >> Signed-off-by: VIET NGA DAO > >> > >> --- > >> v5: > >> - Remove Micron support > >> - Add multiple flashes probe failure handle > >> > >> v4: > >> - Add more flash devices support ( EPCQL and Micron) > >> - Remove redundant messages > >> - Change EPCQ_OPCODE_ID to NON_EPCS_OPCODE_ID > >> - Replace get_flash_name to altera_quadspi_scan > >> - Remove clk related parts > >> - Remove altera_quadspi_plat > >> - Change device tree reg name and remove opcode-id > >> > >> v3: > >> - Change altera_epcq driver name to altera_quadspi for more generic name > >> - Implement flash name searching in altera_quadspi.c instead of spi-nor > >> - Edit the altra quadspi info table in spi-nor > >> - Remove wait_til_ready in all read,write, erase, lock, unlock functions > >> - Merge .h and .c into 1 file > >> > >> v2: > >> - Change to spi_nor structure > >> - Add lock and unlock functions for spi_nor > >> - Simplify the altera_epcq_lock function > >> - Replace reg by compatible in device tree > >> --- > >> > >> .../devicetree/bindings/mtd/altera-quadspi.txt | 45 ++ > >> drivers/mtd/spi-nor/Kconfig | 8 + > >> drivers/mtd/spi-nor/Makefile | 1 + > >> drivers/mtd/spi-nor/altera-quadspi.c | 557 > >> > >> ++++++++++++++++++++ drivers/mtd/spi-nor/spi-nor.c > >> | > >> > >> 18 + > >> 5 files changed, 629 insertions(+), 0 deletions(-) > >> create mode 100644 > >> > >> Documentation/devicetree/bindings/mtd/altera-quadspi.txt create mode > >> 100644 drivers/mtd/spi-nor/altera-quadspi.c > >> > >> diff --git a/Documentation/devicetree/bindings/mtd/altera-quadspi.txt > >> b/Documentation/devicetree/bindings/mtd/altera-quadspi.txt new file mode > >> 100644 > >> index 0000000..e1bcf18 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi.txt > >> @@ -0,0 +1,45 @@ > >> +* MTD Altera QUADSPI driver > >> + > >> +Required properties: > >> +- compatible: Should be "altr,quadspi-1.0" > >> +- reg: Address and length of the register set for the device. It > >> contains + the information of registers in the same order as described > >> by reg-names +- reg-names: Should contain the reg names > >> + "avl_csr": Should contain the register configuration base address > >> + "avl_mem": Should contain the data base address > >> +- #address-cells: Must be <1>. > >> +- #size-cells: Must be <0>. > >> +- flash device tree subnode, there must be a node with the following > >> fields: + - compatible: Should contain the flash name: > >> + 1. EPCS: epcs16, epcs64, epcs128 > >> + 2. EPCQ: epcq16, epcq32, epcq64, epcq128, epcq256, epcq512, > >> epcq1024 + 3. EPCQ-L: epcql256, epcql512, epcql1024 > >> + - #address-cells: please refer to /mtd/partition.txt > >> + - #size-cells: please refer to /mtd/partition.txt > >> + For partitions inside each flash, please refer to > >> /mtd/partition.txt + > >> +Example: > >> + > >> + quadspi_controller_0: quadspi@0x180014a0 { > >> + compatible = "altr,quadspi-1.0"; > >> + reg = <0x180014a0 0x00000020>, > >> + <0x14000000 0x04000000>; > >> + reg-names = "avl_csr", "avl_mem"; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + flash0: epcq256@0 { > >> + compatible = "altr,epcq256"; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + partition@0 { > >> + /* 16 MB for raw data. */ > >> + label = "EPCQ Flash 0 raw > >> data"; + reg = <0x0 > >> 0x1000000>; + }; > >> + partition@1000000 { > >> + /* 16 MB for jffs2 data. > >> */ + label = "EPCQ Flash 0 > >> JFFS 2"; + reg = <0x1000000 > >> 0x1000000>; + }; > > > > IIRC, encoding partitions into OF is deprecated (and it shouldn't be > > part of the example anyway, so please remove this bit). > > > >> + }; > >> + }; //end quadspi@0x180014a0 (quadspi_controller_0) > > > > Remove this incorrect comment. > > > > > > [...] > > Do you mean the partition part below should not be in example? > partition@0 { > /* 16 MB for raw data. */ > label = "EPCQ Flash 0 raw > data"; reg = <0x0 0x1000000>; }; > partition@1000000 { > /* 16 MB for jffs2 data. */ > label = "EPCQ Flash 0 JFFS 2"; > reg = <0x1000000 0x1000000>; > }; Yes, it's not really relevant. > >> +static struct flash_device flash_devices[] = { > >> + FLASH_ID("epcs16", EPCS_OPCODE_ID, 0x14), > >> + FLASH_ID("epcs64", EPCS_OPCODE_ID, 0x16), > >> + FLASH_ID("epcs128", EPCS_OPCODE_ID, 0x18), > >> + > >> + FLASH_ID("epcq16", NON_EPCS_OPCODE_ID, 0x15), > >> + FLASH_ID("epcq32", NON_EPCS_OPCODE_ID, 0x16), > >> + FLASH_ID("epcq64", NON_EPCS_OPCODE_ID, 0x17), > >> + FLASH_ID("epcq128", NON_EPCS_OPCODE_ID, 0x18), > >> + FLASH_ID("epcq256", NON_EPCS_OPCODE_ID, 0x19), > >> + FLASH_ID("epcq512", NON_EPCS_OPCODE_ID, 0x20), > >> + FLASH_ID("epcq1024", NON_EPCS_OPCODE_ID, 0x21), > >> + > >> + FLASH_ID("epcql256", NON_EPCS_OPCODE_ID, 0x19), > >> + FLASH_ID("epcql512", NON_EPCS_OPCODE_ID, 0x20), > >> + FLASH_ID("epcql1024", NON_EPCS_OPCODE_ID, 0x21), > >> + > >> +}; > > > > I think it'd be better to wait until the IP block is fixed and can > > issue READID correctly. > > The hardware IP fix will take time while there are lot of customer are > waiting for this driver. Does that justify pushing serious crap into mainline Linux in any way ? > That is why we decided to upstream the > driver. If the hardware fix, there might not need to have any changes > in driver to support or if yes, it will be just minor. If the hardware can do proper READID opcode, this entire nonsense table will go away and a proper integration into the SPI NOR framework will take place. You might consider submitting this driver for staging, but I definitely am not a big fan of that. > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c > >> b/drivers/mtd/spi-nor/spi-nor.c index 14a5d23..2ab7279 100644 > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -687,6 +687,24 @@ static const struct spi_device_id spi_nor_ids[] = { > >> > >> { "cat25c09", CAT25_INFO( 128, 8, 32, 2, SPI_NOR_NO_ERASE | > >> > >> SPI_NOR_NO_FR) }, { "cat25c17", CAT25_INFO( 256, 8, 32, 2, > >> SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, { "cat25128", CAT25_INFO(2048, 8, > >> 64, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, + > >> + /* Altera EPCQ/EPCS Flashes are non-JEDEC */ > > > > Are they really ? Last time I checked on CV SoC, I was able to read their > > JEDEC ID just fine ; though it's true I used that EPCS core. > > > >> + { "epcs16", INFO(0, 0, 0x10000, 32, 0) }, > >> + { "epcs64", INFO(0, 0, 0x10000, 128, 0) }, > >> + { "epcs128", INFO(0, 0, 0x40000, 256, 0) }, > >> + > >> + { "epcq16", INFO(0, 0, 0x10000, 32, 0) }, > >> + { "epcq32", INFO(0, 0, 0x10000, 64, 0) }, > >> + { "epcq64", INFO(0, 0, 0x10000, 128, 0) }, > >> + { "epcq128", INFO(0, 0, 0x10000, 256, 0) }, > >> + { "epcq256", INFO(0, 0, 0x10000, 512, 0) }, > >> + { "epcq512", INFO(0, 0, 0x10000, 1024, 0) }, > >> + { "epcq1024", INFO(0, 0, 0x10000, 2048, 0) }, > >> + > >> + { "epcql256", INFO(0, 0, 0x10000, 512, 0) }, > >> + { "epcql512", INFO(0, 0, 0x10000, 1024, 0) }, > >> + { "epcql1024", INFO(0, 0, 0x10000, 2048, 0) }, > >> + > >> > >> { }, > >> > >> }; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/