From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sun, 20 Oct 2019 21:14:52 -0600 Subject: [U-Boot] [PATCH v2 14/38] spi: Add support for memory-mapped flash In-Reply-To: References: <20190925141147.191166-1-sjg@chromium.org> <20190925141147.191166-15-sjg@chromium.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Bin, On Sun, 20 Oct 2019 at 20:29, Bin Meng wrote: > > Hi Simon, > > On Sat, Oct 19, 2019 at 4:37 AM Simon Glass wrote: > > > > Hi Bin, > > > > On Fri, 18 Oct 2019 at 09:38, Bin Meng wrote: > > > > > > Hi Simon, > > > > > > On Fri, Oct 18, 2019 at 10:14 PM Simon Glass wrote: > > > > > > > > Hi Bin, > > > > > > > > On Thu, 17 Oct 2019 at 20:32, Bin Meng wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Fri, Oct 18, 2019 at 10:22 AM Simon Glass wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass wrote: > > > > > > > > > > > > > > Hi Vignesh, > > > > > > > > > > > > > > On Wed, 16 Oct 2019 at 04:28, Vignesh Raghavendra wrote: > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > On 12/10/19 10:03 AM, Bin Meng wrote: > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > On Sat, Oct 12, 2019 at 11:08 AM Simon Glass wrote: > > > > > > > > >> > > > > > > > > >> Hi Bin, > > > > > > > > >> > > > > > > > > >> On Wed, 9 Oct 2019 at 07:55, Bin Meng wrote: > > > > > > > > >>> > > > > > > > > >>> Hi Simon, > > > > > > > > >>> > > > > > > > > >>> On Wed, Sep 25, 2019 at 10:12 PM Simon Glass wrote: > > > > > > > > >>>> > > > > > > > > >>>> On x86 platforms the SPI flash can be mapped into memory so that the > > > > > > > > >>>> contents can be read with normal memory accesses. > > > > > > > > >>>> > > > > > > > > >>>> Add a new SPI flash method to find the location of the SPI flash in > > > > > > > > >>>> memory. This differs from the existing device-tree "memory-map" mechanism > > > > > > > > >>>> in that the location can be discovered at run-time. > > > > > > > > >>>> > > > > > > > > > > > > > > > > Whats is the usecase? Why can't spi_flash_read() be used instead? > > > > > > > > Flash + Controller driver can underneath take care of using memory > > > > > > > > mapped mode to read data from flash right while making sure that access > > > > > > > > is within valid window? > > > > > > > > > > > > > > I can see spi_flash_read_dm() but it does not support returning a > > > > > > > pointer to the data, only reading it. > > > > > > > > > > > > > > Also I cannot find any documentation about any of this. I've been > > > > > > > looking in the doc/ directory. > > > > > > > > > > > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > > > > > > in and out buffer pointers so I don't know how to use them. > > > > > > > > > > > > > > Is there an API missing or just comments/documentation? > > > > > > > > > > > > Apart from this I have found that the ich_spi driver does not work for > > > > > > APL since it apparently only supports 'hardware sequencing'. I did try > > > > > > getting software sequencing to work, but no dice. > > > > > > > > > > > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > > > > > > > > > > > So I think it might be best to have two SPI drivers for x86 - one for > > > > > > software sequencing and one for hardware? > > > > > > > > > > So if this is true, I think we can create Kconfig options (HW_SEQ and > > > > > SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig > > > > > file. > > > > > > > > I think at least for TPL I'm going to need to bypass all the code as > > > > it is just too big for TPL. What do you think? > > > > > > So what about other boards/drivers? Say we have drv_foo.c, is the best > > > practice to create a separate drv_foo_tpl.c for use with TPL? > > > > Yes, although in general we can export the functions from the core > > driver, so the TPL one becomes pretty bare-bones - e.g. it handles > > platdata but uses the operations of the core drivers. > > > > > > > > > > > > > Should we have a separate HW SEQ driver? If you look at the HW SEQ > > > > driver I have, it is very little code. > > > > > > > > Or are you saying we should have a combined driver with Kconfig > > > > options to enable either or both of SW SEQ and HW SEQ? > > > > > > > > > > Yes I was suggesting enable either or both SW and HW SEQ in the same driver. > > > > OK I will take a look at that. > > > > My code-size concern still stands though, but let's see how it looks. > > > > > > > > > One other wrinkle is that I have to have a separate driver anyway, > > > > since of-platdata requires it. It is not possible to use of-platdata > > > > in a generic driver since the struct definitions rely on the > > > > compatible string. > > > > > > > > > > > > > > But I see from the Linux kernel driver, it has: > > > > > > > > > > 300static int intel_spi_init(struct intel_spi *ispi) > > > > > 301{ > > > > > 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; > > > > > 303 int i; > > > > > 304 > > > > > 305 switch (ispi->info->type) { > > > > > 306 case INTEL_SPI_BYT: > > > > > 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; > > > > > 308 ispi->pregs = ispi->base + BYT_PR; > > > > > 309 ispi->nregions = BYT_FREG_NUM; > > > > > 310 ispi->pr_num = BYT_PR_NUM; > > > > > 311 ispi->swseq_reg = true; > > > > > 312 > > > > > 313 if (writeable) { > > > > > 314 /* Disable write protection */ > > > > > 315 val = readl(ispi->base + BYT_BCR); > > > > > 316 if (!(val & BYT_BCR_WPD)) { > > > > > 317 val |= BYT_BCR_WPD; > > > > > 318 writel(val, ispi->base + BYT_BCR); > > > > > 319 val = readl(ispi->base + BYT_BCR); > > > > > 320 } > > > > > 321 > > > > > 322 ispi->writeable = !!(val & BYT_BCR_WPD); > > > > > 323 } > > > > > 324 > > > > > 325 break; > > > > > 326 > > > > > 327 case INTEL_SPI_LPT: > > > > > 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; > > > > > 329 ispi->pregs = ispi->base + LPT_PR; > > > > > 330 ispi->nregions = LPT_FREG_NUM; > > > > > 331 ispi->pr_num = LPT_PR_NUM; > > > > > 332 ispi->swseq_reg = true; > > > > > 333 break; > > > > > 334 > > > > > 335 case INTEL_SPI_BXT: > > > > > 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; > > > > > 337 ispi->pregs = ispi->base + BXT_PR; > > > > > 338 ispi->nregions = BXT_FREG_NUM; > > > > > 339 ispi->pr_num = BXT_PR_NUM; > > > > > 340 ispi->erase_64k = true; > > > > > 341 break; > > > > > > > > > > So for INTEL_SPI_BXT (which is for ApolloLake I believe) > > > > > ispi->swseq_reg is not set to true which means it uses hardware > > > > > sequencer, which seems to contradict with what you found. > > > > > > > > I found that it only supports hardware sequencing, so this matches. > > > > > > OK, I see. So the BXT only supports HW SEQ while LPT and BYT support > > > both SW and HW SEQ. > > > > OK, ta. How do I look up what LPT and BYT are? > > > > I think LPT stands Linx Point (the PCH chipset that desktop processors > use) and BYT stands BayTrail which U-Boot already supports. OK ta. BTW I'm just preparing the new series. It will be v3 as I have brought in some patches from the other series which was v2. Regards, Simon