-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Vignesh R Sent: Tuesday, December 18, 2018 10:49 PM To: Jagan Teki jagan@amarulasolutions.com Cc: Marek Vasut marex@denx.de; Tom Rini trini@konsulko.com; U-Boot- Denx u-boot@lists.denx.de; Michal Simek michal.simek@xilinx.com; Siva Durga Prasad Paladugu sivadur@xilinx.com; Boris Brezillon boris.brezillon@bootlin.com; Miquel Raynal miquel.raynal@bootlin.com; Stefan Roese sr@denx.de; Jagan Teki jagan@openedev.com Subject: Re: [U-Boot] [PATCH 00/16] SF: Migrate to Linux SPI NOR framework
On 18-Dec-18 6:02 PM, Jagan Teki wrote:
On Sat, Dec 15, 2018 at 9:13 PM Vignesh R vigneshr@ti.com wrote:
- For BAR support, lets place it as it is and support via spi-nor
Problem is, it not desirable to use BAR as default because its not stateless and does not work with all flash parts. OTOH, it seems like 4 byte addressing (stateless dedicated opcode or with enter/exit 4 byte mode) seems to be standard. Also, Linux doesn't support BAR and haven't seen any request for BAR support. Why support additional feature and burden of maintaining when it may not be needed.
But if you insist, I just have to add BAR support back.
It's not about insistence, ie how we support since from long and u-boot bootloader project does in general. bootloader can be some certain boot difference functionalities unlike Linux, it's better not to compare u-boot with Linux in all cases.
Example we have SPI_TX_BYTE which usually not supported in Linux. Since it's ich controller specific and it require for bootloader to do byte tx on that specific controller, so we supported. Same for the case with the BAR, this specific controller(or supported boards) has been in U-Boot since from long and they do upstream well. So we need to support BAR in any case or we can find any alternative to work the same functionalities. (we tried before but ended-up adding BAR)
How about this instead?
Lets use 4 byte addressing opcodes as default for >16MB flashes. But if CONFIG_SPI_FLASH_BAR is enabled then, spi-nor layer will use BAR registers for >16MB access instead of 4 byte addressing.
Yes, this can be normal approach. but what I'm trying to initiate here is if we have a generic kind of sanity check transfer for > 16MB flashes, then we can notify the controllers enable BAR to support > 16MiB.
I guess you are saying to attempt 4 byte addressing transfers and if that fails then try using BAR? AFAIK, there is _no way_ to detect whether or not controller/flash supports 4 byte addressing and then do a fallback.
This way we can reduce the ifdefs on code, and if we handle BAR in better way to minimize the code eventually we even drop the CONFIG option.
I will remove SPI_FLASH_BAR from defconfigs from all boards expect those controllers that really cannot handle 4 byte addressing. From a quick glance it looks following controllers support only 3 byte addresses: stm32_qspi.c ich.c fsl_qspi.c renesas_rpc_spi.c mtk_qspi.c
I'm not sure whether all these support 4-byte addressing or not?
Just to be clear, all SPI drivers _except_ those in above list can support 4 byte addressing mode w/o any additional code change.
fsl_qspi.c: should support 4 byte as well, since the current frame work had limitation, u-boot.denx.de version of this driver was only supporting until 16MB
Regards Ashish
since we don't have 4-byte on spi-flash they do use BAR for accessing
16MiB, zynq_qspi is the key taker for this initially.
I am confused here, let me put down more details:
Access >16MB of flash memory is possible by 3 ways:
- using dedicated 4 byte addressing opcodes 2. Send "enter 4 byte" command
and then use normal commands but with 4 bytes of address 3 Provide BA24 BA25 of address via BAR register
AFAIK, all flashes support 1 or 2. But only some flashes support 3. So, the idea is to use option 1 or 2 as default and use 3 only if SPI_FLASH_BAR is enabled.
Ideally, there is _nothing_ in SPI controllers that would stop us from using 4 byte addressing because a SPI controller would have no idea of slave connected (ie. it may or may not be a flash). SPI drivers should _blindly_ send/receive dout/din buffers passed as part of spi_xfer() and not interpret these buffers as cmd/addr/data separately So, the decision on which of the above 3 options to use should be in the SPI NOR layer and has nothing to do with SPI controller drivers.
But since spi_flash.c was always used 3 byte addressing, _some_ of the SPI controller drivers are _hard-coded_ to expect 3 byte addresses (eg.: above 4 drivers that I mentioned) See [1] on how these drivers convert dout buffer is back to flash offset. This is because there is no way to communicate cmd/addr/data separately using spi_xfer(), but _intelligent_ SPI HW do need this information. This is not a HW limitation but more of a framework limitation.
So, thus we can classify SPI controllers into roughly two types:
- Controllers that need to know flash specific information like opcode, address
length, dummy bytes etc (controllers that supports memory mapped access to flash(or such other acceleration interface). Such controllers should move to spi-mem (but can optionally implement spi_xfer to support non flash devices)
- Traditional SPI controllers that are usually FIFO/shift register based and
provide direct access to SPI bus will continue to use traditional SPI framework.
Moving to 4 byte addressing mode will not affect second type of SPI controllers, but will break first type of controllers that do their own address conversion logic like[1]. So my suggestion is to enable CONFIG_SPI_FLASH_BAR only for controllers to do[1] until moved to spi-mem.
From my analysis:
stm32_qspi.c ich.c fsl_qspi.c renesas_rpc_spi.c mtk_qspi.c ti_qspi.c cadence_quadspi.c
are candidates to move to spi-mem layer. Rest seems to be traditional SPI controllers.
Yes fsl_qspi.c should be moved to spi-mem layer. Similar work in being done in Linux as well.
Regards Ashish
AFAIU, zynq_qspi seems to be FIFO based SPI controller. Why would zynq_qspi driverneed to know whether SPI NOR layer uses BAR or not? Or did I miss something?
So, except for boards with above controllers, I will remove SPI_FLASH_BAR for all other boards. If there is a regression, then such boards can go back to enabling CONFIG_SPI_FLASH_BAR.
I think we can go the BAR code in flash as you mentioned with CONFIG option, and don't remove any CONFIG items from board configs then we can add sanity check patch on top of spi-nor changes and give warning to boards which never need BAR(ask them to drop CONFIG_BAR). This way transition look promising to move BAR to 4-byte addressing.
Are you suggesting board users should individually remove CONFIG_SPI_FLASH_BAR from defconfigs based on their judgement? How do you propose to do sanity check? Why not use 4 byte addressing as default and enable CONFIG_SPI_FLASH_BAR only for controllers that need them?
AFAIU, above controller HWs(except ich perhaps) can support 4 byte addressing but would need to move to spi-mem.
t Even if we move to spi-mem, but BAR need to handle it flash side. isn't it?
Yes, presence/configuration of BAR registers is abstracted by SPI NOR and spi- mem drivers need not have any knowledge.
[1] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.b ootlin.com%2Fu- boot%2Flatest%2Fsource%2Fdrivers%2Fspi%2Frenesas_rpc_spi.c%23L264&am p;data=02%7C01%7CAshish.Kumar%40nxp.com%7Cd535d3bc013949ad15230 8d6650d0bfa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63680 7504073361903&sdata=DpdMcJ%2F81jOOVE5MRNe6xSqgAlRGXVyvFb6C dm3ot8Q%3D&reserved=0 https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.b ootlin.com%2Fu-boot%2Fv2019.01- rc2%2Fsource%2Fdrivers%2Fspi%2Fmtk_qspi.c%23L253&data=02%7C01 %7CAshish.Kumar%40nxp.com%7Cd535d3bc013949ad152308d6650d0bfa%7 C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636807504073361903 &sdata=JprAHMDaR4DY2vZocYIP0Wfu1l9yuxrl4zveN69hVA0%3D&re served=0
Regards Vignesh _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.de nx.de%2Flistinfo%2Fu- boot&data=02%7C01%7CAshish.Kumar%40nxp.com%7Cd535d3bc013949 ad152308d6650d0bfa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% 7C636807504073361903&sdata=bJBeSCBf%2BtAcF3XyUr04jB1ba4UMTS ZC%2FRSlr6qKkcA%3D&reserved=0