Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753800AbbG0SNc (ORCPT ); Mon, 27 Jul 2015 14:13:32 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:45399 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751353AbbG0SNa convert rfc822-to-8bit (ORCPT ); Mon, 27 Jul 2015 14:13:30 -0400 From: Vikas MANOCHA To: Graham Moore CC: Marek Vasut , "linux-mtd@lists.infradead.org" , David Woodhouse , Brian Norris , "linux-kernel@vger.kernel.org" , Alan Tull , Dinh Nguyen , Yves Vandervennet Date: Mon, 27 Jul 2015 20:12:40 +0200 Subject: RE: [PATCH V5 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller. Thread-Topic: [PATCH V5 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller. Thread-Index: AdDIk833krfNMo+sT4KPjt/811o/JAAAoFFw Message-ID: <9026814FBF99304F9FA3AC3FB72F3E2F04919670@SAFEX1MAIL4.st.com> References: <1437758259-28299-1-git-send-email-grmoore@opensource.altera.com> <1437758259-28299-2-git-send-email-grmoore@opensource.altera.com> <55B2D1F9.9040709@st.com> <55B66CEF.7030100@opensource.altera.com> In-Reply-To: <55B66CEF.7030100@opensource.altera.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: acceptlanguage: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-07-27_02:2015-07-27,2015-07-27,1970-01-01 signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6569 Lines: 178 Hi Graham, > -----Original Message----- > From: Graham Moore [mailto:grmoore@opensource.altera.com] > Sent: Monday, July 27, 2015 10:40 AM > To: Vikas MANOCHA > Cc: Marek Vasut; linux-mtd@lists.infradead.org; David Woodhouse; Brian > Norris; linux-kernel@vger.kernel.org; Alan Tull; Dinh Nguyen; Yves > Vandervennet > Subject: Re: [PATCH V5 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI > Flash Controller. > > Hi Vikas, > > On 07/24/2015 07:02 PM, vikasm wrote: > > Hi Graham, > > > > On 07/24/2015 10:17 AM, Graham Moore wrote: > >> Signed-off-by: Graham Moore > >> --- > >> V2: use NULL instead of modalias in spi_nor_scan call > >> V3: Use existing property is-decoded-cs instead of creating duplicate. > >> V4: Support Micron quad mode by snooping command stream for EVCR > >> command and subsequently configuring Cadence controller for quad > mode. > >> V5: Clean up sparse and smatch complaints. Remove snooping of Micron > >> quad mode. Add comment on XIP mode bit and dummy clock cycles. Set > >> up SRAM partition at 1:1 during init. > >> --- > >> arch/arm/boot/dts/socfpga.dtsi | 1 + > >> arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 1 - > >> drivers/mtd/spi-nor/Kconfig | 6 + > >> drivers/mtd/spi-nor/Makefile | 1 + > >> drivers/mtd/spi-nor/cadence-quadspi.c | 1261 > ++++++++++++++++++++++++++ > >> 5 files changed, 1269 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/mtd/spi-nor/cadence-quadspi.c > >> > >> diff --git a/arch/arm/boot/dts/socfpga.dtsi > >> b/arch/arm/boot/dts/socfpga.dtsi index c71a705..e9ecdce 100644 > >> --- a/arch/arm/boot/dts/socfpga.dtsi > >> +++ b/arch/arm/boot/dts/socfpga.dtsi > >> @@ -695,6 +695,7 @@ > >> is-decoded-cs = <1>; > >> fifo-depth = <128>; > >> status = "disabled"; > >> + m25p,fast-read; > > > > This patch is not applicable to l2-mtd master or linus master repo, might > need to rebase it. > > > > Argh, my mistake, these dts files are not supposed to be in the patch. > > ... > > >> +#include > >> + > >> +#define CQSPI_NAME "cadence-qspi" > > > > replace space with tabs. > > > > They *are* tabs in my original patch. Not sure how they got changed to > spaces... > > ... > >> + > >> +#define CQSPI_FIFO_WIDTH 4 > > > > FIFO width could be 4 or 8 etc depending on the SOC, it would be better to > get it from device. > > You can refer to u-boot code for the same. > > > > OK > ... > >> + > >> +#define CQSPI_INDIRECTTRIGGER_ADDR_MASK 0xFFFFF > > > > Mask value of 0xFFFFF is specific to socfpga platform. > > Please refer to u-boot patchset for same discussion. > > > > OK > ... > >> +static void cqspi_fifo_read(void *dest, const void __iomem > *src_ahb_addr, > >> + unsigned int bytes) { > >> + unsigned int temp; > >> + int remaining = bytes; > >> + unsigned int *dest_ptr = (unsigned int *)dest; > >> + > >> + while (remaining >= CQSPI_FIFO_WIDTH) { > >> + *dest_ptr = readl(src_ahb_addr); > >> + dest_ptr++; > >> + remaining -= CQSPI_FIFO_WIDTH; > > > > this logic only works when fifo width is 4 with "unsigned int" data of 4 > bytes. > > It has been corrected in mainline u-boot or in the u-boot patches. > > > > OK > ... > >> +static int cqspi_indirect_read_setup(struct spi_nor *nor, > >> + unsigned int from_addr) { > >> + unsigned int reg; > >> + unsigned int dummy_clk = 0; > >> + struct cqspi_st *cqspi = nor->priv; > >> + void __iomem *reg_base = cqspi->iobase; > >> + unsigned int ahb_phy_addr = cqspi->ahb_phy_addr; > >> + > >> + writel((ahb_phy_addr & CQSPI_INDIRECTTRIGGER_ADDR_MASK), > >> + reg_base + CQSPI_REG_INDIRECTTRIGGER); > >> + writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR); > > > > Base trigger register address (0x1c register) corresponds to the > > address which should be put on AHB bus to handle indirect transfer > triggered before. > > > > To handle indirect transfer we need to issue addresses from (value of > > 0x1c) to (value of 0x1c) + 15*4 ("4" corresponds to size of SRAM location). > > There are no obstacles in issuing const address just equal to 0x1c. > > Important thing to note is that indirect trigger address has nothing > > in common with your physical or mapped NOR Flash address. > > > > Transfer read/write start addresses should be programmed with the > > absolute flash address to be read/written. > > > > OK, I'll add the trigger address to the device tree, etc. You seem to be > concerned about the fact that on Altera SoC, the trigger address and the ahb > address are different. I'm not seeing any problem with that. No problem with different or same trigger address & ahb address. Trigger address should be passed through device tree as it could be different for different SoCs. Masking it with value like 0xf_ffff to make it suitable for one SOC in driver does not seems right approach. > The CPU reads/writes the FIFO using an address, but that is not the same > address that goes on the bus way down in the interconnect. What's wrong > with that? It's the way our SoC works. If we use the trigger address for the > trigger address register, and the ahb address for reading/writing the FIFO, > then it works fine. This implementation is your Soc specific & not generic for the qspi peripheral. In this way, it won't be possible to use this driver for Socs other than altera soc. Rgds, Vikas > > ... > > >> + watermark = cqspi->fifo_depth * CQSPI_FIFO_WIDTH / 2; > >> + writel(watermark, reg_base + > CQSPI_REG_INDIRECTRDWATERMARK); > >> + writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES); > >> + writel(cqspi->fifo_depth - CQSPI_REG_SRAM_RESV_WORDS, > >> + reg_base + CQSPI_REG_SRAMPARTITION); > > > > sram partioning is not needed to be changed at every read/write, it > > should be ok to configure it in the init like divide half for read & half for > write. > > > > OK > ... > > > > Rgds, > > Vikas > > > > Best regards, > Graham -- 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/