From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb0-f169.google.com ([209.85.213.169]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cWR7j-0004Sw-1L for linux-mtd@lists.infradead.org; Wed, 25 Jan 2017 17:12:45 +0000 Received: by mail-yb0-f169.google.com with SMTP id 123so16461307ybe.3 for ; Wed, 25 Jan 2017 09:12:21 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1484949023-2085-1-git-send-email-kdasu.kdev@gmail.com> <3e1f1c8e-4a87-e186-20cb-2ba784bd58d5@denx.de> <06d8264e-5ddb-aa76-a83e-4c2c3af42f08@denx.de> <62931580-f21f-e6a1-8e8b-d3c92195a2e0@denx.de> From: Kamal Dasu Date: Wed, 25 Jan 2017 12:10:40 -0500 Message-ID: Subject: Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer To: Marek Vasut Cc: Kamal Dasu , Michal Suchanek , Cyrille Pitchen , Mark Brown , bcm-kernel-feedback-list , Florian Fainelli , MTD Maling List Content-Type: text/plain; charset=UTF-8 List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, Jan 25, 2017 at 11:39 AM, Marek Vasut wrote: > On 01/25/2017 05:28 PM, Kamal Dasu wrote: >> If the transfers are short and dest buffer or the flash address are >> unaligned. > > That sounds like a DMA problem where you're trying to fall back to PIO ? > >> Also in case of older version of the controller there are >> some address mapping limitations when a transfer crosses 4MB window >> (addr + len). So in such cases need to fallback to normal MSPI >> reads. > > But the driver can also detect this mode of failure before doing the > transfer and call it's internal functions to perform the transfer as > needed, right? > >> One other option is that controller divers implementation of >> bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the >> m25p80_read() can fallback to normal mspi read. > > I'd much rather see the driver handling such detail internally instead > of patching the core code. Moreover, if you patch the core code, the SF > read will go - in case of a failure- all the way through the SPI > framework only to land in the same driver, which doesn't make much sense. Yes this is how the code was organized before when I was making initial commits. However I had to change it so that spi_flash_read() can be exploited based on the review comments. I was handling code internally using spi generic msg handling code for mspi transfer fallback, but I was told that this code did not belong in the controller driver. Hence the current implementation is geared towards using spi transfer_one() in case of mspi transfers, without bothering with how the messages are formed and pumped by the spi layer. If I reorganize I am back to where I was before. Yes the code lands in the same driver but it returns back to the m25p80 and uses the normal mspi reads. > btw please do NOT top-post: > http://www.arm.linux.org.uk/mailinglists/etiquette.php#e3 > Sorry about that, I will make sure from this point forward I do not top-post. >> Kamal >> >> >> >> On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut wrote: >>> On 01/24/2017 12:41 AM, Kamal Dasu wrote: >>>> "ret can never be > 0 , it is only 0 or negative " >>>> >>>> I can fix this. >>>> >>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale. >>>>>>> But still, if your controller driver is buggy, fix the driver, do not >>>>>>> pollute core code with workarounds. If you do support this sort of >>>>>>> accelerated read and it fails, it means something is seriously wrong. >>>>>>> If you need to invoke regular SPI reads to complete under some obscure >>>>>>> circumstances, do it from the driver, not here. >>>>>> >>>>>> I guess the other half of m25p80_read can be factored out and used as >>>>>> fallback from either m25p80_read or the controller driver. >>>>> >>>>> I think I see what you mean, but care to show an RFC patch ? >>>>> >>>>> -- >>>> >>>> Its not the controller driver, but he hardware limitation with older >>>> controller version. I have tried to see how I can do this better, >>>> however when spi_flash_read() is called cannot handle it within my >>>> driver without returning from the function. I went over this with Mark >>>> previously and this current solution seemed reasonable. Any other >>>> solution outside of the generic driver would replicate a lot of code >>>> unnecessarily. >>> >>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read >>> function could be the solution and invoking the second part from the >>> driver if applicable, but this cannot work because the driver does not >>> know when it's interacting with SPI NOR and when with something else . >>> >>> Can you tell me about the conditions under which the bcm controller >>> fails and should fall back to standard spi read ? >>> >>> -- >>> Best regards, >>> Marek Vasut > > > -- > Best regards, > Marek Vasut