From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [PATCH 1/3] spi/qspi: Add memory mapped read support. Date: Tue, 15 Oct 2013 11:01:42 -0700 Message-ID: <20131015180142.GS23337@ld-irv-0074.broadcom.com> References: <52566ACC.1080100@ti.com> <20131010101410.GG21581@sirena.org.uk> <52568AA3.9080203@ti.com> <20131011100839.GA21581@sirena.org.uk> <525CDB77.4040201@ti.com> <20131015111647.GX2443@sirena.org.uk> <525D2BB3.4020705@ti.com> <20131015124656.GM2443@sirena.org.uk> <525D41E2.30206@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Peter Korsgaard , balbi-l0cyMroinI0@public.gmane.org, Mark Brown , "linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , "spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org" , Trent Piepho , dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org To: Sourav Poddar Return-path: Content-Disposition: inline In-Reply-To: <525D41E2.30206-l0cyMroinI0@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: linux-spi.vger.kernel.org Hi Sourav, On Tue, Oct 15, 2013 at 06:53:46PM +0530, Sourav Poddar wrote: > On Tuesday 15 October 2013 06:16 PM, Mark Brown wrote: > >On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote: > >>On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote: > >>>Can you fix this by enabling the clock is enabled when you return the > >>>buffer to the MTD layer and then disabling the clock when the buffer is > >>>released? > >>Sorry, I did not get you here. With memory mapped read, there is no > >>buffer exchanged, everything takes place at the mtd layer only, what gets > >>exchanged is just the memory mapped address. > >The buffer is the memory mapped address - part of getting the address > >should be preparing the hardware for it. > > > >> if (spi->mode&& SPI_RX_MMAP) { > >> printk("memory mapped mode set\n"); > >>- flash->mmap_read = true; > >>+ flash->mmap_read = spi->memory_map; > >So this probably needs to be a function call to get the buffer (and a > >corresponding one to free it). > So, the flow can be something like this: > > drivers/mtd/devices/m25p80.c > get_flash_buf() > { > lock(); > > t[0] = GET_BUFFER; > t[1] = buf; > ...... > > spi_sync(); > > unlock(); > } > > mtd_read > { > get_flash_buf(); > > if (flash->buf) { > memcpy(); > return 0; > } > } > > Not sure, if free buf is needed as devm_* variant is used to allocate that > memory. I believe you are misplacing the discussion of devm_* variants. devm_* is only useful for resources allocated/mapped released/unmapped at probe and release time. They do not magically remove the burden of resource management for I/O and other dynamic operations. In this case, you are not working at probe time, and you are not actually allocating any memory--your 'get_flash_buf()' and corresponding 'release_flash_buf()' would not be allocating memory but would be ensuring that the HW and driver is in the correct state. Brian ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ie0-x22e.google.com ([2607:f8b0:4001:c03::22e]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VW8wd-0000Bk-51 for linux-mtd@lists.infradead.org; Tue, 15 Oct 2013 18:02:11 +0000 Received: by mail-ie0-f174.google.com with SMTP id qd12so10710280ieb.5 for ; Tue, 15 Oct 2013 11:01:47 -0700 (PDT) Date: Tue, 15 Oct 2013 11:01:42 -0700 From: Brian Norris To: Sourav Poddar Subject: Re: [PATCH 1/3] spi/qspi: Add memory mapped read support. Message-ID: <20131015180142.GS23337@ld-irv-0074.broadcom.com> References: <52566ACC.1080100@ti.com> <20131010101410.GG21581@sirena.org.uk> <52568AA3.9080203@ti.com> <20131011100839.GA21581@sirena.org.uk> <525CDB77.4040201@ti.com> <20131015111647.GX2443@sirena.org.uk> <525D2BB3.4020705@ti.com> <20131015124656.GM2443@sirena.org.uk> <525D41E2.30206@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <525D41E2.30206@ti.com> Cc: Peter Korsgaard , balbi@ti.com, Mark Brown , "linux-mtd@lists.infradead.org" , "spi-devel-general@lists.sourceforge.net" , Trent Piepho , dwmw2@infradead.org List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Sourav, On Tue, Oct 15, 2013 at 06:53:46PM +0530, Sourav Poddar wrote: > On Tuesday 15 October 2013 06:16 PM, Mark Brown wrote: > >On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote: > >>On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote: > >>>Can you fix this by enabling the clock is enabled when you return the > >>>buffer to the MTD layer and then disabling the clock when the buffer is > >>>released? > >>Sorry, I did not get you here. With memory mapped read, there is no > >>buffer exchanged, everything takes place at the mtd layer only, what gets > >>exchanged is just the memory mapped address. > >The buffer is the memory mapped address - part of getting the address > >should be preparing the hardware for it. > > > >> if (spi->mode&& SPI_RX_MMAP) { > >> printk("memory mapped mode set\n"); > >>- flash->mmap_read = true; > >>+ flash->mmap_read = spi->memory_map; > >So this probably needs to be a function call to get the buffer (and a > >corresponding one to free it). > So, the flow can be something like this: > > drivers/mtd/devices/m25p80.c > get_flash_buf() > { > lock(); > > t[0] = GET_BUFFER; > t[1] = buf; > ...... > > spi_sync(); > > unlock(); > } > > mtd_read > { > get_flash_buf(); > > if (flash->buf) { > memcpy(); > return 0; > } > } > > Not sure, if free buf is needed as devm_* variant is used to allocate that > memory. I believe you are misplacing the discussion of devm_* variants. devm_* is only useful for resources allocated/mapped released/unmapped at probe and release time. They do not magically remove the burden of resource management for I/O and other dynamic operations. In this case, you are not working at probe time, and you are not actually allocating any memory--your 'get_flash_buf()' and corresponding 'release_flash_buf()' would not be allocating memory but would be ensuring that the HW and driver is in the correct state. Brian