[U-Boot] [PATCH 00/16] SF: Migrate to Linux SPI NOR framework

[Ashish Kumar]

> -----Original Message-----
> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Vignesh R
> Sent: Tuesday, December 18, 2018 10:49 PM
> To: Jagan Teki <jagan at amarulasolutions.com>
> Cc: Marek Vasut <marex at denx.de>; Tom Rini <trini at konsulko.com>; U-Boot-
> Denx <u-boot at lists.denx.de>; Michal Simek <michal.simek at xilinx.com>; Siva
> Durga Prasad Paladugu <sivadur at xilinx.com>; Boris Brezillon
> <boris.brezillon at bootlin.com>; Miquel Raynal <miquel.raynal at bootlin.com>;
> Stefan Roese <sr at denx.de>; Jagan Teki <jagan at 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 at ti.com> wrote:
> >>
> >>>>> 2) 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:
> 1. 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:
> 1. 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)
> 
> 2. 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 at 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