Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase
On 7 Apr 2015, scottw...@freescale.com wrote: > On Tue, 2015-04-07 at 10:06 -0400, Bill Pringlemeir wrote: >> In any case the document has, >> If the NAND flash supports sub-pages, then what can be done is ECC >> codes can be calculated on per-sub-page basis, instead of per-NAND >> page basis. In this case it becomes possible to read and write >> sub-pages independently. >> Probably if we want sub-pages with this controller and hw-ecc, we >> need to use the virtual buffer features of the chip. The controller >> needs an entire current buffer in the SRAM to calculate the hw-ecc to >> write. So even if it writes less than a full page, the entire page >> must be read to calculate the hw-ecc to be written. > That limitation sounds similar to the Freescale NAND controllers that > I'm familiar with (eLBC and IFC). For eLBC we do subpages by just > writing 0xff, because on that controller the ECC of all 0xff is all > 0xff so it doesn't disturb anything. IFC has different ECC algorithms > where that isn't the case, so we disabled subpage write on IFC. > What is the virtual buffer feature? >> I am pretty sure that all controllers that support hw-ecc will need >> to do this. > Not if they can handle doing ECC on a single subpage. [from another thread, but the same subject]. >> The other way to handle things would to be to investigate the >> NFC_CFG[PAGE_CNT] and NFC_SECSZ to have the virtual pages support >> sub-pages. I think the OOB mapping would be non-standard in such >> cases. > Wouldn't that mess up factory bad block markers? All the stuff above is related (afaik). > What is the virtual buffer feature? It splits programming of a flash page into separate buffers. The BCH-HW-ECC is then applied separately to each 'virtual-page' with reduced strength. Section "31.4.6 Organization of the Data in the NAND Flash" of the Vybrid Software RM has details. > Wouldn't that mess up factory bad block markers? I am unsure; certainly they can be read but they might be a data portion of the fourth sub-page depending on the ECC strength selected. There is also a 'NFC_SWAP' register to switch the position of one 16bit index (move OOB-BBT 16bit from data to OOB). I think this can be used. By non-standard, I meant not fitting the current drivers idea of OOB layout. However, it seems like your comment that the ECC must be different per-subpage (and what Artem said in the sub-pages documentation) makes 'virtual buffers' the most promising path for this driver and sub-page support with hw-ecc. As the bit strength is reduced, the amount of corrections/error detection also seems reduced. I think the math would make this the same for everyone? Your question about factory BBT is a good point. Using the 'virtual buffers' feature will complicate the driver code by quite a bit at least from what I could think of and what I see in the MTD tree where I believe similar features are used. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: remove caching of page in buffer
On 7 Apr 2015, ste...@agner.ch wrote: > On 2015-04-07 16:24, Bill Pringlemeir wrote: >> The OOB patch also significantly decreases UbiFS mounting time in >> Linux. I load Linux itself via tftp/network and not using u-boot >> with nand. I guess I should try that. > Is it UBIFS mounting time or is it bad block scanning? Afaik, it > should speed up the latter significantly, but I don't see why it > should speed up the former. It is both, * no changes. [0.840632] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca [0.964838] 0x0104-0x1000 : "root" .124s base ubi0 mount is .833585s * improved READ_OOB [0.638869] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca [0.707994] 0x0104-0x1000 : "root" .069s base ubi0 mount 1/10s reduction. .738204s This is for a 256MB device. The Ubi mount/scan time is not completely in-significant. For instance, here is my last run with improved READ_OOB [ 0.942538] ubi0: attaching mtd3 ... [ 1.680742] ubi0: background thread "ubi_bgt0d" started, PID 104 This is my 'base ubi0 mount' numbers. The time is slightly different than what I recorded previously. I booted several times without 'READ_OOB' and the times were consistently '.83xxS'. It is possible that the initial BBT scan being quicker altered something; so it is not UBI use of OOB. I am not sure. I just noted that it was ??significantly?? different. Regards, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: remove caching of page in buffer
On 7 Apr 2015, ste...@agner.ch wrote: > On 2015-04-02 22:30, Bill Pringlemeir wrote: >> On 2 Apr 2015, ste...@agner.ch wrote: >> [snip] >> I also measured 'write performance' with the mtd_speedtest >> (performing similar patch to the Linux driver) and I see no >> difference. I think a write benchmark is more appropriate to test >> this functionality? While at least it seems that neither read nor >> write is affected by the simplification. > On U-Boot, I just benchmarked the overall boot time since this is most > important for us. I plan to (re-)integrate the changes into the Linux > driver and check the performance again later this week. > Thanks for for the write test. So I can take this as a Ack? Of course, if you want it, Acked-by: Bill Pringlemeir The OOB patch also significantly decreases UbiFS mounting time in Linux. I load Linux itself via tftp/network and not using u-boot with nand. I guess I should try that. > I will send all the NFC changes in one patchset as v2 probably later > today. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase
On 3 Apr 2015, scottw...@freescale.com wrote: > On Fri, 2015-04-03 at 22:28 +0200, Stefan Agner wrote: >> Why not? IMHO, there are valid reason to do it, since we save coping >> data over the bus (we save copying page size - write len of bytes)... > According to http://www.linux-mtd.infradead.org/doc/ubi.html#L_subpage > the motivation for subpages is saving space, not time, and it's only > used for headers (specifically because using subpages may be slower). > So it may not make a huge performance difference either way, even if > subpages are less efficient on this controller -- though it does have > a complexity cost that is higher than with most controllers. It looks > like the space savings is around one page per block. I think that Artem wrote this. I don't believe the document is completely correct; I think he meant that sub-pages is not architected as a speed improvement. For instance, UBI will read both a VID (volume ID) and EB (erase block). As the are combined into one, then if a driver needs to read a full page (like it is the only thing it supports) then it is simple to read a full page with a VID/EB sub-pages. Certainly for mounts with out 'fastmap', this will result in much faster scans and initial UBI/UbiFS mounts? It also saves one page overhead per erase block. So in all cases, sub-pages will optimize flash usage. Probably performance depends on the MTD driver and CPU. > It'd be good to benchmark up front to be sure you're happy with the > speed/space/complexity tradeoff, though, since enabling/disabling > subpage writes breaks UBI compatibility. I think that if you format the UbiFs without sub-pages and then update code to handle sub-pages, you are fine. If you have flash/UBI formatted with sub-pages and then you disable it in the code, the disk is no longer mountable. In any case the document has, If the NAND flash supports sub-pages, then what can be done is ECC codes can be calculated on per-sub-page basis, instead of per-NAND page basis. In this case it becomes possible to read and write sub-pages independently. Probably if we want sub-pages with this controller and hw-ecc, we need to use the virtual buffer features of the chip. The controller needs an entire current buffer in the SRAM to calculate the hw-ecc to write. So even if it writes less than a full page, the entire page must be read to calculate the hw-ecc to be written. I am pretty sure that all controllers that support hw-ecc will need to do this. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] mtd: vf610_nfc: support subpage write
On 3 Apr 2015, ste...@agner.ch wrote: > I will remove the page read on NAND_CMD_SEQIN, since we memcpy the > full page anyway. I also just realized that the page read actually > happens always and hence slows down even full page writes... Yes, I remove this in Linux (4.0) and it corrupted things when writing. I think your previous conclusion about we never use 'write caching' was wrong. This one is for writes, case NAND_CMD_SEQIN: /* Pre-read for partial writes. */ This one is for reads, case NAND_CMD_READ0: The interface between 'nand_base' and the MTD driver is hard to decipher. Does Scott (or anyone) know if there is any documentation on this? Stefan is completely correct that if a full page is being written, then the 'SEQIN' should not read a page. However, I only see 'column' being passed. How is 'SEQIN' and 'PAGEPROG' to detect if a full page is being written or not? The other way to handle things would to be to investigate the NFC_CFG[PAGE_CNT] and NFC_SECSZ to have the virtual pages support sub-pages. I think the OOB mapping would be non-standard in such cases. The buffer management in the driver is most simple in it's current form. The other versions that I found seemed to be buggy to me. However, the current driver doesn't use all of the NFC SRAM buffer space. Btw, the READ_OOB is very nice for Linux as well. It is a much faster mount of UBI/UbiFs as well. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: remove caching of page in buffer
On 2 Apr 2015, ste...@agner.ch wrote: > To improve performance we remember the current page in the buffer > and avoid reading it twice. This implicit page cache increases > complexity while does not increase performance in real world cases. > This patch removes that feature. > --- > As discussed in the other patchset... > http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/215802 > ...I did some performance measurements: > Time to "Starting kernel ..." > - without bad block scan & with UBIFS fastmap: 2.02s > - with bad block scan & with UBIFS fastmap: 3.99s > - without bad block scan & without UBIFS fastmap: 4.42s > - with bad block scan & without UBIFS fastmap: 6.38s > Without page cache (with this patch applied): > Time to "Starting kernel ..." > - without bad block scan & with UBIFS fastmap: 2.02s > - with bad block scan & with UBIFS fastmap: 4.01s > - without bad block scan & without UBIFS fastmap: 4.41s > - with bad block scan & without UBIFS fastmap: 6.39s [snip] I also measured 'write performance' with the mtd_speedtest (performing similar patch to the Linux driver) and I see no difference. I think a write benchmark is more appropriate to test this functionality? While at least it seems that neither read nor write is affected by the simplification. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase
On 31 Mar 2015, scottw...@freescale.com wrote:
> On Tue, 2015-03-31 at 00:24 +0200, Stefan Agner wrote:
>> Actually, I just realized that the driver is not caching writes.
>>
>> switch (command) {
>> case NAND_CMD_PAGEPROG:
>> nfc->page = -1;
>> +vf610_nfc_transfer_size(nfc->regs, nfc->page_sz);
>> vf610_nfc_send_commands(nfc->regs, NAND_CMD_SEQIN,
>> command, PROGRAM_PAGE_CMD_CODE);
>> vf610_nfc_addr_cycle(mtd, column, page);
>> break;
>>
>> The nfc->page = -1 resets the page cache, so the next read triggers a
>> full page read.
Yes, this is correct. I should have looked at the code again. It would
be interesting to see what it would do if we did cache. I know I was
concerned about the upper layers and this caching; Ie, they may
explicitly want to check a physical re-read of a page just after
programming.
>> I will check the performance consequences when disabling cache
>> entirely, and whether it would be possible to implement a OOB only
>> read.
> OK. In the meantime I'll apply this.
The patches are surely an improvement; maybe not perfect. I think this
is a good decision.
On 2015-03-31 00:15, Scott Wood wrote:
> Especially since you'd be doing one write rather than four full-page
> "partial" writes. Surely the bottleneck here is the NAND chip itself,
> not copying data to the buffer?
The AHB bus that the NFC controller is on is relatively slow. Here are
some numbers from 'AN4947-vybrid-bus-architechure',
Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),
First read Subsequent
2858 all caches on
345269no cache, mmu
437371no cache, no mmu
The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
DDR). The AHB will be about four times slower. Also the reads and
writes to the physical NAND must take place serially. Here are the
program page steps.
1. Issue controller Read full page to NFC buffer.
2. Copy update partial page from DDR to NFC buffer.
3. Issue write NAND page.
The alternate,
1. Issue Read full page to NFC buffer.
2. Copy full page to DDR.
3. Update DDR with new data.
4. Copy updated DDR page to NFC buffer.
5. Issue write NAND page.
This involves a lot more bus transactions with the NFC AHB as either the
source and/or destination. To disable the 'cache' (code in vf610_nfc),
we would actually need page program to only be called with full page
data (maybe that is the case?). This is then much better,
1. Copy full page to NFC buffer.
2. Write NAND page.
Probably it would be beneficial to test this in the 'NAND_CMD_SEQIN' and
not issue the 'read'. Unfortunately, I don't think there is a way to
know if a full page is being written from the driver with the current
NAND API? Maybe the upper layer has already read the whole page, so the
NAND_CMD_SEQIN is always called with the page as current. Maybe,
case NAND_CMD_SEQIN: /* Pre-read for partial writes. */
+ /* Already read? */
+ if (nfc->page == page) /* If this is always true, the */
+ return; /* NFC caching does nothing. */
case NAND_CMD_READ0:
column = 0;
- /* Already read? */
- if (nfc->page == page)
- return;
nfc->page = page;
The AHB bus speed is a similar order to the NFC (33 MHz NFC clock versus
66MHz AHB clock) with both single beat after setup; you can actually
clock the NAND faster with out hw ECC with some NAND chips supporting
~50MHz. Initially the NFC clock was under 10MHz; at this frequency NAND
chip timing is quite dominate. After improving the NFC clock, the
actual transfer from/to the main DDR to the NFC buffers actually becomes
significant.
Well, that is my hypothesis. I guess some measurements are always
beneficial. I had measured with/without 'cache' and the results were not
insignificant. I also put 'printk()' to see what pages were being
read/written and it seemed that both 'read followed by write' and 'write
followed by read' were issued quite frequently to the same page. I am
also pretty sure that most systems will be structured like this.
CPU -> L1 -> L2? - High speed bus -> DDR
CPU - Low speed bus -> NFC
So, I don't think that this is Vybrid specific. The PPC, ColdFire, etc
will probably have similar issues. DMA has the same limitations as the
CPU, with setup overhead. Of course, you can parallel the main CPU with
DMA but many systems want the NAND to complete synchronously; especially
u-boot.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase
On 30 Mar 2015, scottw...@freescale.com wrote:
> On Mon, 2015-03-30 at 22:40 +0200, Stefan Agner wrote:
>> However, if removing the caching in the driver would lead to a
>> performance drop, I would rather prefer to keep it...
> What is special about this controller, that caching makes sense here
> but not on other controllers? If it makes sense everywhere, then the
> upper layer is the place to do it.
While I observed some performance differences, but that is an aside. I
think the mxc driver is similar in sub-page (or partial page) support.
Before doing a sub-page write, it reads a full page and then copies the
sub-page update to the buffer to be reprogrammed. This works fine if
each and every sub-page has separate OOB data for software ECC. The
hardware ECC of this vf610_nfc controller doesn't support this.
At least that is my understanding of this [mxc_nand] code,
case NAND_CMD_SEQIN:
if (column >= mtd->writesize) {
/*
* before sending SEQIN command for partial write,
* we need read one page out. FSL NFC does not support
* partial write. It always sends out 512+ecc+512+ecc
* for large page nand flash. But for small page nand
* flash, it does support SPARE ONLY operation.
*/
if (host->pagesize_2k) {
/* call ourself to read a page */
mxc_nand_command(mtd, NAND_CMD_READ0, 0,
page_addr);
}
in 'drivers/mtd/nand/mxc_nand.c'. So, originally this was mainly for
sub-pages (an optimization but also functionality).
So the 'caching' is just to preserve read data before a partial write as
the vf610 requires a full page (like the mxc). The same 'caching' was
keep for 'write followed by read' when the ECC is successful.
I realize that the upper layers may not like this so I mentioned it; I
guess that is proof that the patch is getting a good review and I am not
causing problems for Stefan ;)
I also agree with Stefan that setting the SECSZ in the 2nd patch will
result in less data transfer (and maybe less current/power and system
noise). However, this will be inside the NFC controller. Talking over
the AHB to the NFC controller is quite slow on the Vybrid. Certainly it
seems a little more elegant to tell the controller to transfer nothing
(and that we [programmers] expect nothing as a sort of documentation)?
I think it would be worthwhile to benchmark without the cache. Or maybe
Stefan already has some numbers? Upper layers doing partial pages will
definitely benefit with the 'cache'; we would also need more DDR memory
as the NFC controller memory is being used as a scratch buffer.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase
On 24 Mar 2015, ste...@agner.ch wrote: > The driver tries to re-use the page buffer by storing the page > number of the current page in the buffer. The page is only read > if the requested page number is not currently in the buffer. When > a block is erased, the page number is marked as invalid if the > erased page equals the one currently in the cache. However, since > a erase block consists of multiple pages, also other page numbers > could be affected. > > The commands to reproduce this issue (on a written page): >> nand dump 0x800 >> nand erase 0x0 0x2 >> nand dump 0x800 > > The second nand dump command returns the data from the buffer, > while in fact the page is erased (0xff). > > Avoid the hassle to calculate whether the page is affected or not, > but set the page buffer unconditionally to invalid instead. > > Signed-off-by: Stefan Agner > --- > This are two bug fixes which would be nice if they would still > make it into 2015.04... > > drivers/mtd/nand/vf610_nfc.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/vf610_nfc.c > b/drivers/mtd/nand/vf610_nfc.c index 928d58b..9de971c 100644 --- > a/drivers/mtd/nand/vf610_nfc.c +++ b/drivers/mtd/nand/vf610_nfc.c @@ > -369,8 +369,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, > unsigned command, break; > > case NAND_CMD_ERASE1: - if (nfc->page == page) - nfc->page = > -1; + nfc->page = -1; vf610_nfc_send_commands(nfc->regs, command, > NAND_CMD_ERASE2, ERASE_CMD_CODE); vf610_nfc_addr_cycle(mtd, column, > page); This change looks sensible. It is also possible that because sub-pages were removed that we could just remove the caching all together. It is possible that a higher layer may intentionally want to program and then do a read to verify. I had seen that different FS seem to do 'write' and then immediately follow with a read. If you believe the controller and the write status was ok, then I think it is fine to reuse the existing buffer and keep this caching. For UBI, there were VID/EB header reads when sub-pages were enabled as they are in the same page; but these are now seperate pages. Especially, people may only care about code size in 'u-boot' and the typical use will only be to read an image (or config) from NAND so this optimization is probably not too helpful (execept maybe when flashing new kernels). The 2nd patch set is not needed if we do not re-use existing data? I guess we want to stay the same as the mainline Linux you are submitting. I haven't benchmarked the updates since sub-pages were removed to see if this is worth it. I think it was only ~10-20% in some benchmark I was doing with the 'caching'. At least in the small, this is a minimal change that is correct. Ack-by: Bill Pringlemeir ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] imx: don't clobber reset cause
On 10 Feb 2015, eric.nel...@boundarydevices.com wrote: > I posted a couple of additional options and received no comment > from you. > Neither of them works as-is because of the ordering of events > (print_cpuinfo() is called before restoring the environment), > so your suggestion would require an additional call at startup > which currently doesn't exist across i.MX boards. > The primary argument against the original patch was that > bits **could** accumulate. In practice, I believe this to > be a bit of a red herring, since two bits cover essentially > all of the normal conditions: > bit 0 - power on > bit 4 - watchdog Yes, the normal case is easy. Odd things can happen during software development. We both agree you could miss something; maybe only whether that is important is not clear. People using the CSU to protect errant memory writes to disabled peripherals might be useful. From the imx6 RM, Software has to take care to clear this register by writing one after every reset that occurs so that the register will contain the information of recently occurred reset. > The watchdog flag is set with reboot under Linux and reset > in U-Boot, so we could re-work the switch statement to do > the right thing. In fact, it appears broken now because it > has 0x11 displaying "POR", when I believe that should be > "WDOG". I am pretty sure that the register is full of garbage on a 'POR', so the 'POR' bit overrides everything; at least on the Vybrid. This is why it is important to clear the bits. The imx6DL-RM seems to say the same, When the system comes out of reset, this register will have bits set corresponding to all the reset sources that occurred during system reset. > Other bits could conceivably accumulate, but I don't see > the value of worrying about cases like "JTAG_RESET". > The reason we're pursuing this at all is because we'd like > to know the difference between a restart caused by power > interruption and a system lockup, and we'd like to do > this under Linux or Windows Embedded. Please don't mis-understand. I see value in what you are trying to accomplish. I just want to make sure the information that gets reported is robust and correct. It would probably be nice if the Vybrid followed the same pattern; but maybe they are different? From reading the RMs they seem the same. Regards, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
> On 02/05/2015 03:58 PM, Eric Nelson wrote:
>> The cause of a reset is generally useful, and shouldn't be
>> blindly cleared in the process of displaying it as a part
>> of the boot announcement.
>>
>>
>>
>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
>> index 28ccd29..4a54051 100644
>> --- a/arch/arm/imx-common/cpu.c
>> +++ b/arch/arm/imx-common/cpu.c
>> @@ -28,6 +28,7 @@ char *get_reset_cause(void)
>> {
>> u32 cause;
>> struct src *src_regs = (struct src *)SRC_BASE_ADDR;
>> +char *rval = "unknown";
>>
>> cause = readl(&src_regs->srsr);
>>
>>
>> }
>> +setenv("reset_cause", rval);
>> +return rval;
On 6 Feb 2015, eric.nel...@boundarydevices.com wrote:
> Nak.
> This routine is called before the environment is initialized.
Okay, that makes sense we can't do this.
> There's no way to set the environment here, which I think
> means that this patch is a pre-cursor to anything else.
> http://patchwork.ozlabs.org/patch/436492/
> If we feel the need to reset it before an O/S boots, there is a common
> spot here:
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/imx-common/cpu.c;hb=HEAD#l205
> That won't be invoked if the O/S is started with "go" though
> (often done with QNX or Windows).
The 'write' is to prevent against multiple bits set. Say for instance
you remove this check. Someone has U-boot siting at a command line for
a long time before they boot an OS. Occasionally, some memory/hardware
error happens and the device resets. In this case, the reason will be
from multiple resets.
I think it is good to write/clear (and record) the reason early. So, I
think just having 'cause' as a global or accessible when the environment
is ready would solve your problem.
Writing does two things,
1. clears the 'reset cause' to prevent someone else to look at.
2. primes the 'cause' for the next reset; the bits are sticky. w1c
I think we miss the last point with this patch. The 'cause' just needs to
be recorded and kept around until the environment is up (or however we
want to pass it around).
Recording it early and clearing is best as you can know what the 'last
cause' was. Otherwise, you might get multiple causes in the register.
Err, maybe I mis-understand something? If you do this patch and have
multiple soft resets (watchdog, CSU, JTAG, ipp, warm, etc) are the bits
accumulated?
Fwiw,
Bill.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
On 5 Feb 2015, eric.nel...@boundarydevices.com wrote: [snip] > Of course (if we go that route). > > I thought of this when typing up the list of values for the > commit comment. > > The human readable form is harder to handle on the receiving > side though, which is why I favor hex. I also prefer the hex. I have had very weird values in this register. For a normal user, we can guess and display a string. Sometimes in the case of double/triple resets, there can be very strange values. You can get these when DMA goes crazy (because of bad code or otherwise). Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] imx: don't clobber reset cause
On 4 Feb 2015, eric.nel...@boundarydevices.com wrote:
> The cause of a reset is generally useful, and shouldn't be
> blindly cleared in the process of displaying it as a part
> of the boot announcement.
>
> If a particular system wants to clear it out, this should
> be done later after there's an opportunity for code or
> boot commands to read the value.
>
> Signed-off-by: Eric Nelson
> ---
> arch/arm/imx-common/cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index 28ccd29..3e0a582 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -30,7 +30,6 @@ char *get_reset_cause(void)
> struct src *src_regs = (struct src *)SRC_BASE_ADDR;
>
> cause = readl(&src_regs->srsr);
> - writel(cause, &src_regs->srsr);
>
> switch (cause) {
> case 0x1:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The
write is for a hard power on case where these reason registers are full
of weird bogus values (at least on Vybrid; I suspect on iMx). In the
case of a non-POR, the register bits are good. However, if you don't
clear the status, on the next reset it may have multiple registers bits
even though you really want to know the last reason (bit).
Another option would be to clear the value and store the 'cause'
somewhere for other U-Boot users. Unless you wanted to read this from
an OS? I think both files should behave the same, all else equal.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] arm: relocation: clear .bss section with arch memset if defined
On 2 Feb 2015, bpringlem...@nbsps.com wrote:
> On 31 Jan 2015, albert.u.b...@aribaud.net wrote:
>
>> Hello Przemyslaw,
>>
>> On Wed, 28 Jan 2015 13:55:42 +0100, Przemyslaw Marczak
>> wrote:
>>> For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY,
>>> will highly increase the memset/memcpy performance. This is able
>>> thanks to the ARM multiple register instructions.
>>>
>>> Unfortunatelly the relocation is done without the cache enabled,
>>> so it takes some time, but zeroing the BSS memory takes much more
>>> longer, especially for the configs with big static buffers.
>>>
>>> A quick test confirms, that the boot time improvement after using
>>> the arch memcpy for relocation has no significant meaning.
>>> The same test confirms that enable the memset for zeroing BSS,
>>> reduces the boot time.
>>>
>>> So this patch enables the arch memset for zeroing the BSS after
>>> the relocation process. For ARM boards, this can be enabled
>>> in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.
>
>> Since the issue is that zeroing is done one word at a time, could we
>> not simply clear r3 as well as r2 (possibly even r4 and r5 too) and
>> do a double (possibly quadruple) write loop? That would avoid calling
>> a libc routine from the almost sole file in U-Boot where a C
>> environment is not necessarily granted.
I thought the same thing...
> diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index
> 22df3e5..fab3d2c 100644 --- a/arch/arm/lib/crt0.S +++
> b/arch/arm/lib/crt0.S @@ -115,14 +115,22 @@ here: bl
> c_runtime_cpu_setup /* we still call old routine here */
>
> ldr r0, =__bss_start/* this is auto-relocated! */
> - ldr r1, =__bss_end /* this is auto-relocated! */
>
> +#ifdef CONFIG_USE_ARCH_MEMSET
> + ldr r3, =__bss_end /* this is auto-relocated! */
> + mov r1, #0x /* prepare zero to clear BSS */
> +
> + subsr2, r3, r0 /* r2 = memset len */
> + bl memset
> +#else
> + ldr r1, =__bss_end /* this is auto-relocated! */
> mov r2, #0x /* prepare zero to clear BSS */
>
> clbss_l:cmp r0, r1 /* while not at end of BSS */
> strlo r2, [r0]/* clear 32-bit BSS word */
> addlo r0, r0, #4 /* move to next */
> blo clbss_l
> +#endif
This is great news (increase boot speed). Maybe if this files wasn't
conditional? Assuming the the 'BSS' is aligned (LDS enforced),
ldr r0, =__bss_start/* this is auto-relocated! */
ldr r1, =__bss_end /* this is auto-relocated! */
+mov r2, #0 /* prepare zero to clear BSS */
+mov r3, #0
+mov r4, #0
+mov r5, #0
+mov r6, #0
+mov r7, #0
+mov r8, #0
+mov lr, #0
+b 1f
+.align 4
clbss_l:
+ stmia r0!, {r2-r8,lr}/* clear 32 BSS word */
+ stmia r0!, {r2-r8,lr}/* easy to unroll */
+ 1: cmp r0, r1
blo clbss_l
The code should work on all ARM versions? Then every platform would
benefit. I think the larger portion of the 'ARCH memset' is to handle
alignment issues. Sometimes the tail/head portion can be handled
efficiently with 'strd', etc which is only on some CPUs. It is easy to
setup the BSS so that both the head/tail are aligned but I think the
above code only requires multiples of 32bytes as total BSS size (it is
easy to jump into the middle of an unrolled loop with 'add pc, rn<<2,
#constant'). The size/iteration can be easily tweaked (8/16/32 bytes).
At least in principal *if* there is some size alignment on BSS it is
fairly easy to write some generic ARM to quickly clear the BSS that will
be just as competitive as any ARCH memset version. The above code is
adding about 13 words of code.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [GIT PULL] Zynq SoC changes
On 23 Jan 2015, mon...@monstr.eu wrote: > On 01/23/2015 02:05 AM, Tom Rini wrote: >> I can't find a toolchain that doesn't give me something like: arm: + >> zynq_zc70x +(zynq_zc70x) arch/arm/cpu/armv7/zynq/lowlevel_init.S: >> Assembler messages: +(zynq_zc70x) >> arch/arm/cpu/armv7/zynq/lowlevel_init.S:19: Error: selected processor >> doe s not support ARM mode `fmrx r1,FPEXC' +(zynq_zc70x) >> On Wed, Jan 21, 2015 at 10:38:47AM +0100, Michal Simek wrote: > ok. I see. We have neon instructions enabled by default in xilinx > toolchain. I have sent the patch which create and add > PLATFORM_RELFLAGS += -mfpu=neon to config.mk. > This should solve the problem with compilation. No problem to add > this patch as the first in this serial not to break bisectability of > the tree. I think you can add something like, .fpu neon-vfpv4 # or whatever FPU is appropriate. to the lowlevel_init.S and then you can use the 'fmxr' instruction even without compiler flags (and of course gas needs to know about it). It makes sense if the platform turns on floating point by default and you need to disable it in lowlevel_init.S and don't want anyone else to be using floating point. ... but I am not sure what exactly is going on so maybe it is not a good fit here? Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] ARmv7: Add a soc_init hook to start.S
>> On 21 Jan 2015, hdego...@redhat.com wrote: >> >>> On some SoCs / ARMv7 CPU cores we need to do some setup before >>> enabling the icache, etc. Add a soc_init hook with a weak default >>> which just calls cpu_init_cp15. >>> >>> This way different implementations can be provided to do some extra >>> work before or after cpu_init_cp15, or completely replacing >>> cpu_init_cp15. >>> >>> Signed-off-by: Hans de Goede >>> --- >>> arch/arm/cpu/armv7/start.S | 18 +- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S >>> index fdc05b9..9882b20 100644 >>> --- a/arch/arm/cpu/armv7/start.S >>> +++ b/arch/arm/cpu/armv7/start.S >>>>> -64,7 +64,7 @@ reset: >>> >>> /* the mask ROM code should have PLL and others stable */ >>> #ifndef CONFIG_SKIP_LOWLEVEL_INIT >>> - bl cpu_init_cp15 >>> + bl soc_init >>> bl cpu_init_crit >>> #endif >>> >>>>> -102,6 +102,22 @@ ENDPROC(save_boot_params) >>> /* >>> * + * void soc_init(void) + * __attribute__((weak)); + * + * Stack >>> pointer is not yet initialized at this moment + * Don't save >>> anything to stack even if compiled with -O0 + * + >>> */ >>> +ENTRY(soc_init) >>> + mov r9, lr >>> + bl cpu_init_cp15 >>> + mov pc, r9 @ back to my caller >>> +ENDPROC(soc_init) >>> + .weak soc_init > On 21-01-15 22:59, Bill Pringlemeir wrote: >> >> You could just use a 'tail call' and make this, >> >> +ENTRY(soc_init) >> +b cpu_init_cp15 >> +ENDPROC(soc_init) On 22 Jan 2015, hdego...@redhat.com wrote: > True, although I think that actually saving lr to r9 is useful as an > example for boards who want to override this, and that we can spare > the 2 extra instructions :) Yes, it is a good example if you are looking to over-ride the 'soc_init' and I expect that it was hard to figure out that you needed to save the lr; I can see why you want to warn others. Two instructions isn't bad. However, I looked a the code from a different perspective. I just want to see what is going on in a normal boot. I would look at this code and think what the heck is this about. Especially as 'r9' is used for other purposes. >> Or even as the code follows just add a duplicate label, so 'soc_init' >> is a weak version of 'cpu_init_cp15'? This gives no additional code >> in a final binary. I guess it depends on how the generic 'soc_init' >> might be modified in the future? > I think that having a separate entry for it is much more clear, > otherwise the entire symbol will be hard to find / grok for people > trying to get into the code. Hmm. I understood better after looking at the sunxi implementation. The weak 'soc_init' seemed like I didn't understand something. The saving of 'lr' in 'r9' is only needed if you have extra code and is nothing to do with global data or something. So, it is not just less code but I think it is more clear what the default case is doing. I agree, it is definitely an opinion. Either way works. > Albert what do you want to do here? ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] ARmv7: Add a soc_init hook to start.S
On 21 Jan 2015, hdego...@redhat.com wrote: > On some SoCs / ARMv7 CPU cores we need to do some setup before > enabling the icache, etc. Add a soc_init hook with a weak default > which just calls cpu_init_cp15. > > This way different implementations can be provided to do some extra > work before or after cpu_init_cp15, or completely replacing > cpu_init_cp15. > > Signed-off-by: Hans de Goede > --- > arch/arm/cpu/armv7/start.S | 18 +- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S > index fdc05b9..9882b20 100644 > --- a/arch/arm/cpu/armv7/start.S > +++ b/arch/arm/cpu/armv7/start.S >>> -64,7 +64,7 @@ reset: > > /* the mask ROM code should have PLL and others stable */ > #ifndef CONFIG_SKIP_LOWLEVEL_INIT > - bl cpu_init_cp15 > + bl soc_init > bl cpu_init_crit > #endif > >>> -102,6 +102,22 @@ ENDPROC(save_boot_params) > /* > * >+ * void soc_init(void) >+ *__attribute__((weak)); >+ * >+ * Stack pointer is not yet initialized at this moment >+ * Don't save anything to stack even if compiled with -O0 >+ * >+ */ >+ENTRY(soc_init) >+ mov r9, lr >+ bl cpu_init_cp15 >+ mov pc, r9 @ back to my caller >+ENDPROC(soc_init) >+ .weak soc_init You could just use a 'tail call' and make this, +ENTRY(soc_init) + b cpu_init_cp15 +ENDPROC(soc_init) Or even as the code follows just add a duplicate label, so 'soc_init' is a weak version of 'cpu_init_cp15'? This gives no additional code in a final binary. I guess it depends on how the generic 'soc_init' might be modified in the future? If we put code after the 'cpu_init_cp15' in the generic version, then we should keep saving the 'lr' into 'r9'; also 'cpu_init_cp15' should never use 'r9'. This maybe good to document (or someone may break your sunxi code). >+ >+/* >+ * > * cpu_init_cp15 > * > * Setup CP15 registers (cache, MMU, TLBs). The I-cache is turned on unless ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: fix exception vectors
> On Mon, 19 Jan 2015 11:11:34 +0100, Luca Ellero >> As far as I can see the mechanism to relocate vectors is implemented >> only on iMX25/27 and involves high vectors address (0x). On 19 Jan 2015, albert.u.b...@aribaud.net wrote: > You are correct that the mechanism is /used/ only by mx25 and mx27. > However, it has been introduced to support /all/ SoCs (or even > boards), not only mx25, mx27, and /all/ exception vector handling > scenarios, not only high vectors addresses. > Actually, the standard high and low address is handled by default, > and mx25 and mx27 are cases where this default is overriden /because/ > high or low vector addresses are *unapplicable* for them due to their > memory mapping. > On Mon, 19 Jan 2015 11:11:34 +0100, Luca Ellero >> The problem is that, after relocation, U-Boot doesn't fix the vector >> table addresses, they still point to the old addresses (before >> relocation). This is wrong and this patch fixes them to point to the >> new addresses. On 19 Jan 2015, albert.u.b...@aribaud.net wrote: > You are right about the problem, and this problem is exactly what the > relocate_vectors mechanism is here to fix -- exactly the same problem > mx25 and mx27 had before we fixed it (quite recently actually, as it > was done mid-November, between 2015.01-rc1 and 2015.01-rc1 rc2). > On Mon, 19 Jan 2015 11:11:34 +0100, Luca Ellero >> I had to use this patch trying to implement IRQ support for a >> Freescale iMX6 board. iMX6 does not provide RAM at the high vectors >> address and trying to access them leads to errors, so I couldn't use >> them. On 19 Jan 2015, albert.u.b...@aribaud.net wrote: > This is *exactly* the case where you should define your own version of > relocate_vectors (probably at SoC level). A key difference is the ARM CPU architecture version. The ARMv7/Cortex on the imx6 will support vector table 'remaps'. Currently I see, #ifdef CONFIG_HAS_VBAR /* * If the ARM processor has the security extensions, * use VBAR to relocate the exception vectors. */ ldr r0, [r9, #GD_RELOCADDR] /* r0 = gd->relocaddr */ mcr p15, 0, r0, c12, c0, 0 /* Set VBAR */ #else This should be available for the CPUs, but I don't see this symbol besides a 'Kconfig'. There are three versions on the Cortex, but the VBAR (whether secure or not) is the the correct one. At least in theory, this code if activated should work for the iMx6 and an override of the 'weak' relocate vectors is not needed. I think trying to use the 'VBAR' is the correct way to go? Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Unreadable UBIFS partition after power cuts
On 16 Jan 2015, anton.habeg...@delta-es.com wrote: > What does a "dangling branch" and "dangling match" mean? Are those > situations handled differently under U-Boot? ... I don't know about this. However, it is easy to think that the issue is with the UbiFs layer as it doesn't mount. However, it can be several layer; the MTD driver, MTD, UBI and UbiFS. I guess you have some raw image captured? If you have a raw NAND image you can try different analysis. Here is a nandsim mount to emulate on a PC. # nandsim with Micron 3.3V 256MiB, 2048 bytes page. modprobe nandsim first_id_byte=0x2c second_id_byte=0xda third_id_byte=0x90 fourth_id_byte=0x95 parts=2,64,64 flash_erase /dev/mtd3 0 0 ubiformat /dev/mtd3 -f rootfs.ubi # rootfs.ubi is the flash dump #modprobe ubi mtd=3 #mount -t ubifs -o ro /dev/ubi0_3 /mnt/ubifs You can put the flash id sequence for your boards flash in the 'id_byte' and setup different partitions to emulate your actual device (ie, your parts=... is different). The last two lines will mount the partition using the Linux PC code. Grab 'git://git.infradead.org/mtd-utils.git'. There is a file in ubi-utils called ubinfo.c and it maybe helpful. Also, hujian yang has been trying to get a 'ubidump' program merged to mtd-utils. See: http://lists.infradead.org/pipermail/linux-mtd/2014-December/056828.html http://lists.infradead.org/pipermail/linux-mtd/2014-December/056829.html and many others. The program might be helpful. I think there aren't functional issue with the current patches, but structural issues (fitting with mtd-utils). In this message I attached a much simpler parser of just the UBI layer (this is really dumb, but should be easy to modify). http://lists.infradead.org/pipermail/linux-mtd/2014-July/054712.html They might be useful to identify injured blocks/pages. Obviously, the 'recovery needed' shows that fixing some partial write/erase is the issue. If UBI passes a damaged page/info to UbiFS, then it will act on bad info. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: vf610: fix boot from SD-card
On 8 Jan 2015, ste...@agner.ch wrote:
> Boot from SD-card (and probably also from NAND) was broken since
> commit d6d07a9bec ("arm: vf610: add NAND support for vf610twr").
It was broke before that when fsl_qspi was added? Or at least that also
added a lot of code. It maybe dependant on the compiler (and hence
binary size).
> It looks like the increased size of U-Boot lead to a situation where
> the boot ROM overwrote its own stack/heap while loading U-Boot from
> the SD-card to the SRAM.
I didn't think of this. That is a possibility.
> However, U-Boot worked fine when loaded through USB serial loader
> directly into SRAM. It looks like loading from SD-card uses other
> stack/heap location then the serial loader (or maybe no stack or heap
> at all).
I have tried various u-boot images with git-bisect. For instance when
the QSPI was added, I had issues. It seems that an image near 220k was
a threshold. The offset in the On-chip SRAM is 0x8000 (from the 'ld'
file) and the imximage prints '3f008000'. So, I think that the offset
takes the breaking point to where we cross from OC-SRAM0 to OC-SRAM1; I
guess a stack maybe located here but it must be fairly small. I thought
that maybe the HAB has a mapping of device memory start/size and didn't
like it when we crossed the boundary.
> This fix moves U-Boot to gfxRAM, which is 512kB in size and is not
> used by the boot ROM nor the SD-card loader of it.
Whatever the issue is, this fixes it for me as well. I would guess that
NAND boot is also not possible or probably anything but serial-download
modes.
Thanks,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: vf610: fix boot from SD-card
On 8 Jan 2015, ste...@agner.ch wrote:
> Boot from SD-card (and probably also from NAND) was broken since
> commit d6d07a9bec ("arm: vf610: add NAND support for vf610twr").
> It looks like the increased size of U-Boot lead to a situation
> where the boot ROM overwrote its own stack/heap while loading
> U-Boot from the SD-card to the SRAM. However, U-Boot worked fine
> when loaded through USB serial loader directly into SRAM. It
> looks like loading from SD-card uses other stack/heap location
> then the serial loader (or maybe no stack or heap at all).
> This fix moves U-Boot to gfxRAM, which is 512kB in size and is not
> used by the boot ROM nor the SD-card loader of it.
>
> Signed-off-by: Stefan Agner
> ---
> We use U-Boot from gfxRAM on Vybrid since quite a while for the
> Colibri VF50/VF61 modules in our downstream U-Boot branch. Hence
> this is quite well tested.
>
> Just today I discovered that this is broken on the Tower module.
> Back then, when I made the offending change, I only tested on the
> Tower board using the serial loader, since I thought the behaviour
> should be exactly the same. But now, I know better... :-)
>
> If still possible, it would be great to get this into v2015.01...
>
> include/configs/vf610twr.h | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/configs/vf610twr.h b/include/configs/vf610twr.h
> index 6fd0b17..bd79e81 100644
> --- a/include/configs/vf610twr.h
> +++ b/include/configs/vf610twr.h
> @@ -125,7 +125,10 @@
> #define CONFIG_BOOTDELAY 3
>
> #define CONFIG_LOADADDR 0x8200
> -#define CONFIG_SYS_TEXT_BASE 0x3f008000
> +
> +/* We boot from the gfxRAM area of the OCRAM. */
> +#define CONFIG_SYS_TEXT_BASE 0x3f408000
> +#define CONFIG_BOARD_SIZE_LIMIT 524288
>
> #define CONFIG_EXTRA_ENV_SETTINGS \
> "script=boot.scr\0" \
Acked-by: Bill Pringlemeir
See also:
http://lists.denx.de/pipermail/u-boot/2014-December/197641.html
It seems since v2014.04 u-boot on the Vybrid Tower SD card has not
booted. Most users will probably boot this way.
https://community.freescale.com/docs/DOC-95248
Thanks,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] mx25: Fix boot hang by avoiding vector relocation
> On Tue, Jan 06, 2015 at 01:06:48PM -0200, Fabio Estevam wrote:
>> From: Fabio Estevam
>>
>> Since commit 3ff46cc42b9d73d0 ("arm: relocate the exception vectors")
>> mx25pdk hangs like this:
>>
>> CPU: Freescale i.MX25 rev1.2 at 399 MHz
>> Reset cause: WDOG
>> Board: MX25PDK
>> I2C: ready
>> DRAM: 64 MiB
>> (hangs)
>>
>> Add a specific relocate_vectors macro that skips the vector
>> relocation, as the i.MX25 SoC does not provide RAM at the high
>> vectors address (0x), and (0x) maps to ROM.
>>
>> This allows mx25 to boot again.
>>
>> Signed-off-by: Fabio Estevam
On 8 Jan 2015, tr...@ti.com wrote:
> I'd like to pull this in for the release. I'd also really like
> someone else to ack or otherwise comment on this (and why this is more
> right than not just relocating the vectors as v1 did, I see both boot
> to a U-Boot prompt but shouldn't we do a bit more testing to confirm
> that we don't need to relocate these exception vectors or have we now
> introduced some subtle breakage (or perhaps not so subtle once you hit
> it) in these cases? Thanks!
Acked-By: Bill Pringlemeir
The addresses in v1 of the patch are for the imx27. The will do nothing
for the imx25. On the imx25, the address 0x0 is ROM and will BUS error
on a write (default without any patch). The address 0x is
mapped to a WEIM peripheral (external Addr+Data w chip select) and
writes there will not BUS error (so v1 patch works). However, it is
misleading as the real 'hook' vectors are located in IRAM (0x78xx);
it is very similar in concept to imx27. It is better just to provide a
stub that does nothing than misleading people to think they are hooked.
The HAB code on the iMX25 ROM has some vectors installed so that any
errors will reset or go to serial boot mode. This is what this platform
has done all along. The most correct way would be to hook the vectors,
but the hook addresses are only in an NDA doc and would require some
testing, etc. because even that document is not 100% clear.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] scripts: fix binutils-version.sh for 'as' without a package.
On 7 Jan 2015, yamad...@jp.panasonic.com wrote: > Thanks for your patch. > > I think this works but could it be more simplified? > > In your commit-log, you mentioned only some of tools provide > additional information surrounded by brackets. > > If so, we can > [1] remove blackets > [2] and then take the first word that consists of numbers+period > > > Like this: > > version_string=$($gas --version | head -1 | \ > sed -e 's/(.*)//' -e 's/[^0-9.]*\([0-9.]*\).*/\1/') > > MAJOR=$(echo $version_string | cut -d . -f 1) > MINOR=$(echo $version_string | cut -d . -f 2) > > printf "%02d%02d\\n" $MAJOR $MINOR I realized I didn't need to spawn sed twice, but removing the '(package)' stuff seems better. Thanks for giving me a reason to fix the whitespace. Also the string, GNU assembler version 2.24.0-6.fc21 20140613 Was originally reporting '2014061320140613' prior to your version. The new version reports '0224' like all the others. Fwiw, Bill. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3] scripts: fix binutils-version.sh for 'as' without a package.
Commit 73c25753 fixed the common issue that binutil packages (tool/organization
that packaged or built the bin-utils) are included in brackets and this may
falsely be recognized as a version. However, some tools do not provide a
'package' and previously we add the 'Gnu assembler..' to the version.
Strip out the '(package version text)' and then look for a ##.## string.
Signed-off-by: Bill Pringlemeir
---
scripts/binutils-version.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh
index 0bc26cf..a343681 100755
--- a/scripts/binutils-version.sh
+++ b/scripts/binutils-version.sh
@@ -14,7 +14,8 @@ if [ ${#gas} -eq 0 ]; then
exit 1
fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *\([0-9.]*\).*/\1/' )
+version_string=$($gas --version | head -1 | \
+ sed -e 's/(.*)//; s/[^0-9.]*\([0-9.]*\).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1)
MINOR=$(echo $version_string | cut -d . -f 2)
--
1.8.0.2
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] scripts: fix binutils-version.sh for 'as' without a package.
Commit 73c25753 fixed the common issue that binutil packages (tool/organization
that packaged or built the bin-utils) are included in brackets and this may
falsely be recognized as a version. However, some tools do not provide a
'package' and previously we add the 'Gnu assembler..' to the version.
Run a 2nd pass on the 'version_string' to strip off any leading characters when
a package is not provided in brackets.
Signed-off-by: Bill Pringlemeir
---
scripts/binutils-version.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh
index 0bc26cf..b1afc98 100755
--- a/scripts/binutils-version.sh
+++ b/scripts/binutils-version.sh
@@ -5,7 +5,6 @@
# Prints the binutils version of `gas-command' in a canonical 4-digit form
# such as `0222' for binutils 2.22
#
-
gas="$*"
if [ ${#gas} -eq 0 ]; then
@@ -15,6 +14,7 @@ if [ ${#gas} -eq 0 ]; then
fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *\([0-9.]*\).*/\1/' )
+version_string=$(echo "$version_string" | sed -e 's/[^0-9]*\([0-9.]*\).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1)
MINOR=$(echo $version_string | cut -d . -f 2)
--
1.8.0.2
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] scripts: fix binutils-version.sh for 'as' without a package.
Commit 73c25753 fixed the common issue that binutil packages (tool/organization
that packaged or built the bin-utils) are included in brackets and this may
falsely be recognized as a version. However, some tools do not provide a
'package' and previously we add the 'Gnu assembler..' to the version.
Run a 2nd pass on the 'version_string' to strip off any leading characters when
a package is not provided in brackets.
Signed-off-by: Bill Pringlemeir
---
scripts/binutils-version.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh
index 0bc26cf..68cd0fe 100755
--- a/scripts/binutils-version.sh
+++ b/scripts/binutils-version.sh
@@ -5,7 +5,7 @@
# Prints the binutils version of `gas-command' in a canonical 4-digit form
# such as `0222' for binutils 2.22
#
-
+set -x
gas="$*"
if [ ${#gas} -eq 0 ]; then
@@ -15,6 +15,7 @@ if [ ${#gas} -eq 0 ]; then
fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *\([0-9.]*\).*/\1/' )
+version_string=$(echo "$version_string" | sed -e 's/[^0-9]*\([0-9.]*\).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1)
MINOR=$(echo $version_string | cut -d . -f 2)
--
1.8.0.2
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] v2015.01-rc4 REGRESSION: "scripts: fix binutils-version.sh" breaks things for non Linaro toolchains
>> On Tue, Jan 06, 2015 at 11:27:43AM +0100, Hans de Goede wrote:
>>> ping ? current master still has this regression, it is not fatal,
>>> but it is not
>>> pretty either.
> On 6 Jan 2015, tr...@ti.com wrote:
>> Did you see my earlier reply? It's OK with vanilla toolchains (see
>> ELDK) and Linaro ones, but breaking on how Fedora mangles the version
>> string. I'm _not_ saying it's a Fedora problem, but can you poke at
>> this a bit? If not, I'll play with echo and see if I can do it.
On 6 Jan 2015, bpringlem...@nbsps.com wrote:
> diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh
> index 0bc26cf..955267d 100755 --- a/scripts/binutils-version.sh +++
> b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils
> version of `gas-command' in a canonical 4-digit form # such as `0222'
> for binutils 2.22 # - gas="$*"
>
> if [ ${#gas} -eq 0 ]; then
> @@ -14,9 +13,9 @@ if [ ${#gas} -eq 0 ]; then
> exit 1
> fi
>
> -version_string=$($gas --version | head -1 | sed -e 's/.*)
> *\([0-9.]*\).*/\1/' ) +version_string=$($gas --version | head -1 | sed
> -e 's/.*) *\([0-9\.]*\).*/\1/' )
>
> -MAJOR=$(echo $version_string | cut -d . -f 1)
> -MINOR=$(echo $version_string | cut -d . -f 2)
> +MAJOR=$(echo "$version_string" | cut -d . -f 1)
> +MINOR=$(echo "$version_string" | cut -d . -f 2)
>
> -printf "%02d%02d\\n" $MAJOR $MINOR
> +printf "%02d%02d\\n" "$MAJOR" "$MINOR"
>
>
> The main issue is quoting of the 'sed' expression. We had regex
> [0-9.], but we want [0-9\.] so that we match a literal '.' as opposed
> to anything. Or so I speculate. I made a script that output the
> version info Hans has and after the patch it is fine.
Err none of the above. The RedHat binutils doesnt have a 'package'
string. With brackets. For example,
GNU assembler (GNU Binutils for Ubuntu) 2.24
GNU assembler (crosstool-NG 1.20.0 - nbsps) 2.24
It is just,
GNU assembler version 2.24.0-6.fc21 20140613
---
++
Below gets the first ##.## after a bracket or the first ##.## No
escaping of the . is needed inside the range of course.
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh
index 0bc26cf..b1afc98 100755
--- a/scripts/binutils-version.sh
+++ b/scripts/binutils-version.sh
@@ -5,7 +5,6 @@
# Prints the binutils version of `gas-command' in a canonical 4-digit form
# such as `0222' for binutils 2.22
#
-
gas="$*"
if [ ${#gas} -eq 0 ]; then
@@ -15,6 +14,7 @@ if [ ${#gas} -eq 0 ]; then
fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *\([0-9.]*\).*/\1/' )
+version_string=$(echo "$version_string" | sed -e 's/[^0-9]*\([0-9.]*\).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1)
MINOR=$(echo $version_string | cut -d . -f 2)
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] v2015.01-rc4 REGRESSION: "scripts: fix binutils-version.sh" breaks things for non Linaro toolchains
On 6 Jan 2015, tr...@ti.com wrote:
> On Tue, Jan 06, 2015 at 11:27:43AM +0100, Hans de Goede wrote:
>> Hi all,
>>
>> ping ? current master still has this regression, it is not fatal,
>> but it is not
>> pretty either.
>
> Did you see my earlier reply? It's OK with vanilla toolchains (see
> ELDK) and Linaro ones, but breaking on how Fedora mangles the version
> string. I'm _not_ saying it's a Fedora problem, but can you poke at
> this a bit? If not, I'll play with echo and see if I can do it.
I did a shell check on this script,
shellcheck -f gcc binutils-version.sh
binutils-version.sh:13:9: warning: Don't use variables in the printf format
string. Use printf "..%s.." "$foo". [SC2059]
binutils-version.sh:19:14: note: Double quote to prevent globbing and word
splitting. [SC2086]
binutils-version.sh:20:14: note: Double quote to prevent globbing and word
splitting. [SC2086]
binutils-version.sh:22:22: note: Double quote to prevent globbing and word
splitting. [SC2086]
binutils-version.sh:22:29: note: Double quote to prevent globbing and word
splitting. [SC2086]
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh
index 0bc26cf..955267d 100755
--- a/scripts/binutils-version.sh
+++ b/scripts/binutils-version.sh
@@ -5,7 +5,6 @@
# Prints the binutils version of `gas-command' in a canonical 4-digit form
# such as `0222' for binutils 2.22
#
-
gas="$*"
if [ ${#gas} -eq 0 ]; then
@@ -14,9 +13,9 @@ if [ ${#gas} -eq 0 ]; then
exit 1
fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *\([0-9.]*\).*/\1/' )
+version_string=$($gas --version | head -1 | sed -e 's/.*) *\([0-9\.]*\).*/\1/'
)
-MAJOR=$(echo $version_string | cut -d . -f 1)
-MINOR=$(echo $version_string | cut -d . -f 2)
+MAJOR=$(echo "$version_string" | cut -d . -f 1)
+MINOR=$(echo "$version_string" | cut -d . -f 2)
-printf "%02d%02d\\n" $MAJOR $MINOR
+printf "%02d%02d\\n" "$MAJOR" "$MINOR"
The main issue is quoting of the 'sed' expression. We had regex [0-9.],
but we want [0-9\.] so that we match a literal '.' as opposed to
anything. Or so I speculate. I made a script that output the version
info Hans has and after the patch it is fine.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] mx25pdk does not boot with 3ff46cc42b9d7
On 5 Jan 2015, feste...@gmail.com wrote: > I noticed that mx25pdk (ARM926) does not boot anymore with top of tree > U-boot. > > Doing a git bisect resulted in the following commit as being the > guilty one: > > commit 3ff46cc42b9d73d01c86df904425704410958470 > Author: Georges Savoundararadj > Date: Tue Oct 28 23:16:11 2014 +0100 > > arm: relocate the exception vectors > > This commit relocates the exception vectors. > As ARM1176 and ARMv7 have the security extensions, it uses VBAR. For > the other ARM processors, it copies the relocated exception vectors to > the correct address: 0x or 0x. > > Signed-off-by: Georges Savoundararadj > Acked-by: Albert ARIBAUD > Cc: Tom Warren > > Any ideas? $ git show 3ff46cc42b9d73d01c86df9044257 /* * Default/weak exception vectors relocation routine * * This routine covers the standard ARM cases: normal (0x), * high (0x) and VBAR. SoCs which do not comply with any of * the standard cases must provide their own, strong, version. */ The code looks correct. However, the imx25 has the HAB and some default vectors set up. Do you assume they overwrite the HAB vectors? For the imx25, the 'V bit = 0' for the physical HAB ROM location, but it can be remapped to 0x. However, there is nothing there (0x) initially and this only makes sense with the MMU enabled. I am not sure what happened before; why it worked. Maybe you could define an empty relocate_vectors() in the imx25 board file and see if everything is ok? I don't think that a write to the ROM code will BUS error? If a write BUS errors, then the empty routine will work/boot. However, u-boot will not be handling the vectors unless we hook in the IRAM at 0x7801, where I guess the ROM code (at address zero) branches too. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 0/2] arm: vf610: Change Vybrid Tower boot memory.
Without the second patch, U-Boot will not boot on the Vybrid tower systems for me. A better mechanism would be to use the SPL to load direct to DDR3. However, the gfxRAM is large enough to fit the current (~400KB) image for initial load. U-Boot is relocated to DDR3 when initialized, so the life in gfxRAM is small. I also discovered some redundant defines in the same file. include/configs/vf610twr.h | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 2/2] arm: vf610: Initial load to 1/2MB OC gfxRAM.
The gfxRAM has no ECC, but it is 512KB in size. The other ECC SRAM comes in two banks of 256K each. The HAB uses parts of the 2nd bank and it seems that loading from the SD card may not exceed a bank size (256k-32k boot offset = 224k). U-boot since v2014.04 are larger than this and do not boot by default. As U-Boot relocates to DDR3 the gfxRAM is probably reliable enough. However, an SPL framework would be better. Signed-off-by: Bill Pringlemeir --- include/configs/vf610twr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/configs/vf610twr.h b/include/configs/vf610twr.h index 62ffb9e..41ad478 100644 --- a/include/configs/vf610twr.h +++ b/include/configs/vf610twr.h @@ -122,7 +122,7 @@ #define CONFIG_BOOTDELAY 3 #define CONFIG_LOADADDR0x8200 -#define CONFIG_SYS_TEXT_BASE 0x3f008000 +#define CONFIG_SYS_TEXT_BASE 0x3f408000 #define CONFIG_EXTRA_ENV_SETTINGS \ "script=boot.scr\0" \ -- 1.8.0.2 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/2] arm: vf610: Remove duplicate MTD defines.
Some MTD defines are repeated twice; once with UBI and then with MTD. Remove the duplicate MTD defines from the UBI grouping. Signed-off-by: Bill Pringlemeir --- include/configs/vf610twr.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/configs/vf610twr.h b/include/configs/vf610twr.h index 6fd0b17..62ffb9e 100644 --- a/include/configs/vf610twr.h +++ b/include/configs/vf610twr.h @@ -60,11 +60,8 @@ /* UBI */ #define CONFIG_CMD_UBI #define CONFIG_CMD_UBIFS -#define CONFIG_CMD_MTDPARTS #define CONFIG_RBTREE #define CONFIG_LZO -#define CONFIG_MTD_DEVICE -#define CONFIG_MTD_PARTITIONS /* Dynamic MTD partition support */ #define CONFIG_CMD_MTDPARTS -- 1.8.0.2 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] arm: build arch memset/memcpy in Thumb2 mode
> > diff --git a/arch/arm/config.mk b/arch/arm/config.mk > index f0eafd6..14f4e9f 100644 > --- a/arch/arm/config.mk > +++ b/arch/arm/config.mk > @@ -26,7 +26,9 @@ PLATFORM_CPPFLAGS += -D__ARM__ > > # Choose between ARM/Thumb instruction sets ifeq > ($(CONFIG_SYS_THUMB_BUILD),y) -PF_CPPFLAGS_ARM := $(call cc-option, > -mthumb -mthumb-interwork,\ +AFLAGS_IMPLICIT_IT := $(call > as-option,-Wa$(comma)-mimplicit-it=always) +PF_CPPFLAGS_ARM := > $(AFLAGS_IMPLICIT_IT) $(AFLAGS_NOWARN) \ + $(call cc-option, -mthumb I don't think we need the '$(AFLAGS_NOWARN)' as you fixed that? I removed it during my tests and didn't see anything. Especially all builds produced output. I guess this is copy/paste from a Linux Makefile as I don't even see where it is set in U-Boot. > -mthumb-interwork,\ $(call cc-option,-marm,)\ $(call > cc-option,-mno-thumb-interwork,)\ ) diff --git > a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h > index 5e4789b..11b80fb 100644 --- a/arch/arm/include/asm/assembler.h > +++ b/arch/arm/include/asm/assembler.h @@ -14,12 +14,14 @@ * assembler > source. */ [snip] Tested-by: Bill Pringlemeir diff --git a/arch/arm/config.mk b/arch/arm/config.mk index f113c8b..0667984 100644 --- a/arch/arm/config.mk +++ b/arch/arm/config.mk @@ -27,7 +27,7 @@ PLATFORM_CPPFLAGS += -D__ARM__ # Choose between ARM/Thumb instruction sets ifeq ($(CONFIG_SYS_THUMB_BUILD),y) AFLAGS_IMPLICIT_IT := $(call as-option,-Wa$(comma)-mimplicit-it=always) -PF_CPPFLAGS_ARM := $(AFLAGS_IMPLICIT_IT) $(AFLAGS_NOWARN) \ +PF_CPPFLAGS_ARM := $(AFLAGS_IMPLICIT_IT) \ $(call cc-option, -mthumb -mthumb-interwork,\ $(call cc-option,-marm,)\ $(call cc-option,-mno-thumb-interwork,)\ diff --git a/include/configs/vf610twr.h b/include/configs/vf610twr.h index 6fd0b17..8bd7d14 100644 --- a/include/configs/vf610twr.h +++ b/include/configs/vf610twr.h @@ -46,8 +46,8 @@ #undef CONFIG_CMD_IMLS /* NAND support */ -#define CONFIG_CMD_NAND -#define CONFIG_CMD_NAND_TRIMFFS +/* #define CONFIG_CMD_NAND */ +/* #define CONFIG_CMD_NAND_TRIMFFS */ #ifdef CONFIG_CMD_NAND #define CONFIG_NAND_VF610_NFC @@ -105,7 +105,7 @@ #define CONFIG_PHY_MICREL /* QSPI Configs*/ -#define CONFIG_FSL_QSPI +/* #define CONFIG_FSL_QSPI */ #ifdef CONFIG_FSL_QSPI #define CONFIG_CMD_SF ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Attached DTB no longer supported u-boot.imx?
> On Fri, Nov 28, 2014 at 06:56:32PM -0500, Bill Pringlemeir wrote: >> Sorry, both trees behave the same. I will switch to separate >> binaries. On 28 Nov 2014, tr...@ti.com wrote: > This is not intentional. I suspect I know what commit did this. Can > you please do a git bisect from v2015.01-rc1 to -rc2 and see what it > says the bad commit is? Thanks! I have the 'ext4-div' patch and I must reduce the vf610 image size in order to boot (why I have 'git reset --hard'). git reset --hard; git bisect bad HEAD is now at c6150aa image-fdt: boot_get_fdt() return value when no DTB exists c6150aaf2f2745141a7c2ceded58d7efbfeace7d is the first bad commit commit c6150aaf2f2745141a7c2ceded58d7efbfeace7d Author: Noam Camus Date: Wed Oct 22 17:17:49 2014 +0300 image-fdt: boot_get_fdt() return value when no DTB exists I believe that when no DTB is around we should return 1. This why I fixed such scenarious to not return zero anymore. Else kernel might get NULL pointer to DTB which doesn't exists. Signed-off-by: Noam Camus :04 04 f343f802d1491dc54545271694669c21f5efaa65 1120435d7aee7a9c12a6c8e2c0b19a364cbaf185 M common If I take rc2, revert c6150aaf2 and rebuild then the attached DTB image seems fine for booting. Regards, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Attached DTB no longer supported u-boot.imx?
On 28 Nov 2014, bpringlem...@nbsps.com wrote: > > I tried to update the new u-boot I get, > > Bytes transferred = 3413041 (341431 hex) > Kernel image @ 0x8600 [ 0x00 - 0x33e150 ] > Could not find a valid device tree [snip] > Is the imx just using newer patches to be merged to the main tree and > I must switch to a detached DT? Or is there something else? Sorry, both trees behave the same. I will switch to separate binaries. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Attached DTB no longer supported u-boot.imx?
I tried to update the new u-boot I get,
Bytes transferred = 3413041 (341431 hex)
Kernel image @ 0x8600 [ 0x00 - 0x33e150 ]
Could not find a valid device tree
The 'bootcmd' is,
tftp ${loadaddr} zImage.dtb; bootz ${loadaddr}
has the syntax changed and I am not up to date? I think I saw something
about an attached DTB not supported.
u-boot-imx$ git describe
v2009.08-18413-g5c84206
err, that is no good. I have 2a82ec77d27ef5f860a107c4b764643a655dceeb
as 'Prepare v2015.01-rc2' and I had to apply 5c84206 to compile with a
4.9 hard-float ARM compiler.
I pulled the main 'u-boot.git' and this does not happen.
u-boot$ git describe
v2015.01-rc2-87-g0f89149
It is '38cd8c4253013ccdd4052ee021f6066fe9a52551', with the same patch as
'pwclient git-am 415336'
Is the imx just using newer patches to be merged to the main tree and I
must switch to a detached DT? Or is there something else?
Thanks,
Bill.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds
> On 2014-11-20 20:14, Jeroen Hofstee wrote:
>> Ok thanks for digging that up, that doesn't sound like a problem
>> then. Stefan, can you check if you can actually fix the warnings
>> instead of suppressing them?
On 21 Nov 2014, ste...@agner.ch wrote:
> Ok, I could apply the changes from your patch and it fixed the
> warnings in memset.S. However, when I build the file in ARM mode then
> (without CONFIG_SYS_THUMB_BUILD set). I get this:
> arch/arm/lib/memset.S: Assembler messages: arch/arm/lib/memset.S:92:
> Error: bad instruction `stmiage ip!,{r1,r3-r8,lr}'
I think you need '.syntax unified' if you want those in ARM mode. I
guess you found that out too? I see,
+ .syntax unified
+#ifdef CONFIG_SYS_THUMB_BUILD
+ .thumb
+ .thumb_func
+#endif
in '[PATCH v2] arm: build arch memset/memcpy in Thumb2 mode'; so this
solved it?
I think it is a very nice feature to have Thumb2 on Vybrid. Many boot
devices may have limited bandwidth compared to the running system.
Thanks for your work.
Regards,
Bill Pringlemeir
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds
>>> --- >>> arch/arm/lib/memset.S | 40 >>> 1 file changed, 20 insertions(+), 20 deletions(-) >>> >>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S >>> index 0cdf895..4fe38f6 100644 >>> --- a/arch/arm/lib/memset.S >>> +++ b/arch/arm/lib/memset.S >>>>> -18,8 +18,8 @@ >>> 1: subsr2, r2, #4 @ 1 do we have enough >>> blt 5f @ 1 bytes to align with? >>> cmp r3, #2 @ 1 >>> - strltb r1, [r0], #1@ 1 >>> - strleb r1, [r0], #1@ 1 >>> + strblt r1, [r0], #1@ 1 >>> + strble r1, [r0], #1@ 1 >> To test this, can we just use 'objdump'. The hex codes should be >> identical; there is only one encoding. It should produce the same >> binaries. No need to run test-suites, etc. On 20 Nov 2014, jer...@myspectrum.nl wrote: > yes, I should be trivial to test (and find the trivial problem, with > the patch I attached). I am wondering though if all version of > gas accept the suffix notation... any idea? One part of the answer is here, https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD The 'strCCb' version is definitely more popular in older ARM books. Certainly there could be bugs and/or patched versions that make a difference. Probably it would be helpful to know what versions are supported. Back in 1999 it seems that the code at least tries to take conditions anywhere, https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099 I think it is most likely to result in a parse error if it wasn't supported. Any version since Thumb2/Unified (2003-2005?) was introduced should be accepting this syntax with less issues. Ie, it seems like a better way forward. Historical versions are here, http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/ Who knows if some vendor patched things to mess something up? Probably grabbing an older 'gas' version and verifying it was the same binary before/after the patch would probably be fair confirmation? I don't think you can 100% guarantee this doesn't break with some archaic vendors gas. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Query on CONFIG_SYS_THUMB_BUILD
> On Wed, 19 Nov 2014 13:34:34 -0500, Bill Pringlemeir > wrote: >>>>> In message <20141119074214.3d414ce6@lilith> Albert wrote: >>>>>> For -mauto-it, it is not documented in the gas documentation >>>>>> online or in my current as' --target-help. I'll dig this deeper >>>>>> today, but barring any scream from me, the change above is fine >>>>>> globally in U-Boot. >>>> On 19 Nov 2014, w...@denx.de wrote: >> >>>>> Apparently this [1] is where it is coming from; no further >>>>> documentation there, though. >>>> >>>>> [1] https://sourceware.org/ml/binutils/2009-05/msg00132.html >> >>> On Wed, 19 Nov 2014 11:31:05 -0500, Bill Pringlemeir >> >>>> I would think that if this worked they would make it automatic and >>>> not an option. Probably this is only in certain binutils/as. >>>> With 4.6.3 and 4.9.1 I do not have this option, >> On 19 Nov 2014, albert.u.b...@aribaud.net wrote: >>> Which option do you mean? -mimplicit-it or -mauto-it? >> '-mauto-it' , which I think if it is working correctly would be >> rolled into '-mimplicit-it' as it generates better code (for an >> assembler :). I followed the thread above and the patch originator >> says he needs to fix section issues and the 'command line options' >> and he would follow up the proposed patch. On 20 Nov 2014, albert.u.b...@aribaud.net wrote: > I am getting lost, even when reading (quickly, I admit) the patch that > adds it; I don't see what -mauto-it does exactly. Can you summarize > and clarify the effects of -mimplicit-it (I guess I know this one but > it's never a bad thing to get a second opinion), -mauto-it and their > interaction? I guess you know how the 'IT' works. The Ubuntu/Debian people give a good explanation in a few paragraphs, https://wiki.ubuntu.com/ARM/Thumb2PortingHowto#Conditional_Execution My trying to explain this may have confused thing... Here is Wolfgang's reference, https://sourceware.org/ml/binutils/2009-05/msg00132.html Here is a 2nd reference, https://sourceware.org/ml/binutils/2009-06/msg00162.html Originally Daniel Gutson used '-mauto-it' and then it was converted to '-mimplicit-it'. I am not sure if '-mauto-it' exists in the wild. I have never heard of that option before seeing this email thread. Also my assembler says, Assembler messages: Error: unrecognized option -mauto-it I have built with the most recent binutils, gcc4.9.1 using crosstool-ng. Maybe only some non-mainline tools picked up this '-mauto-it' patch. I don't think it hurts to support '-mauto-it', but an assembler test should be done to see if it accepts the option. hth, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds
> On 20-11-14 13:15, Stefan Agner wrote: >> No particular reason, I did not know how to fix this without digging >> into it. Hence, after I discovered this, I checked why those warnings >> do not happen for the kernel, then I applied just the AFLAGS the >> kernel is using. I guess fixing the underlying issue is the better >> option, and doing this also for the kernel would be the best >> way... Maybe the kernel community also knows better why they choose >> to use the AFLAGS instead (and if there are gas version which do have >> problems with a proper fix)... On 20 Nov 2014, jer...@myspectrum.nl wrote: > for what it is worth, I have attached patch hanging around, but I > never actually tested it. It is for the current version. >> From c151254b3de49d8fccb69ab4f9442d884b9ff85c Mon Sep 17 00:00:00 >> 2001 > From: Jeroen Hofstee > Date: Thu, 20 Nov 2014 14:06:26 +0100 > Subject: [PATCH] arm: memset: make it UAL compliant > --- > arch/arm/lib/memset.S | 40 > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S > index 0cdf895..4fe38f6 100644 > --- a/arch/arm/lib/memset.S > +++ b/arch/arm/lib/memset.S > @@ -18,8 +18,8 @@ > 1:subsr2, r2, #4 @ 1 do we have enough > blt 5f @ 1 bytes to align with? > cmp r3, #2 @ 1 > - strltb r1, [r0], #1@ 1 > - strleb r1, [r0], #1@ 1 > + strblt r1, [r0], #1@ 1 > + strble r1, [r0], #1@ 1 To test this, can we just use 'objdump'. The hex codes should be identical; there is only one encoding. It should produce the same binaries. No need to run test-suites, etc. Fwiw, Bill. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Query on CONFIG_SYS_THUMB_BUILD
>>> In message <20141119074214.3d414ce6@lilith> you^H^H^H Stefan wrote: >>>> For -mauto-it, it is not documented in the gas documentation online >>>> or in my current as' --target-help. I'll dig this deeper today, but >>>> barring any scream from me, the change above is fine globally in >>>> U-Boot. >> On 19 Nov 2014, w...@denx.de wrote: >>> Apparently this [1] is where it is coming from; no further >>> documentation there, though. >> >>> [1] https://sourceware.org/ml/binutils/2009-05/msg00132.html > On Wed, 19 Nov 2014 11:31:05 -0500, Bill Pringlemeir >> I would think that if this worked they would make it automatic and >> not an option. Probably this is only in certain binutils/as. >> >> With 4.6.3 and 4.9.1 I do not have this option, On 19 Nov 2014, albert.u.b...@aribaud.net wrote: > Which option do you mean? -mimplicit-it or -mauto-it? '-mauto-it' , which I think if it is working correctly would be rolled into '-mimplicit-it' as it generates better code (for an assembler :). I followed the thread above and the patch originator says he needs to fix section issues and the 'command line options' and he would follow up the proposed patch. I guess at some version each and every 'xx' was converted to 'it xx\n' where is some conditional instruction. For the patch above, '-mauto-it' teaches the assembler to glob them together into 'itet...' type conditions. The Thumb2 supports up to four conditions (and negated condition) instructions. On my version of the tools (I think it is gcc; but maybe binutils), if I use '-mauto-it' it gives an unknown option error. I think that Linux does a probe of this feature and passes it (-mauto-it) if the assembler accepts it. So, if we add to u-boot we should probably take care that the ARM 'as' can take the option. I also see posts on the web of people complaining of this option in other code bases. Fwiw, Bill. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Query on CONFIG_SYS_THUMB_BUILD
On 19 Nov 2014, w...@denx.de wrote: > Dear Albert, > > In message <20141119074214.3d414ce6@lilith> you wrote: >> >> For -mauto-it, it is not documented in the gas documentation online >> or in my current as' --target-help. I'll dig this deeper today, but >> barring any scream from me, the change above is fine globally in >> U-Boot. > Apparently this [1] is where it is coming from; no further > documentation there, though. > [1] https://sourceware.org/ml/binutils/2009-05/msg00132.html I would think that if this worked they would make it automatic and not an option. Probably this is only in certain binutils/as. With 4.6.3 and 4.9.1 I do not have this option, [foo.S] .syntax unified .thumb foo: cmp r0, #5 movne r1,#6 moveq r1,#2 bx lr bar: cmp r0, #10 movhi r1,#3 movls r1,#7 moveq r1,#11 bx lr $ arm-none-linux-gnueabi.gcc4.6.3/bin/arm-none-linux-gnueabi-as -mcpu=cortex-a5 -mimplicit-it=always foo.S -o foo.o $ arm-none-linux-gnueabi-vybrid-4.9.1/bin/arm-none-linux-gnueabi-as -mcpu=cortex-a5 -mimplicit-it=always foo.S -o foo.o Both give 'objdump -S foo.o', foo.o: file format elf32-littlearm Disassembly of section .text: : 0: 2805cmp r0, #5 2: bf14ite ne 4: 2106movne r1, #6 6: 2102moveq r1, #2 8: 4770bx lr 000a : a: 280acmp r0, #10 c: bf8cite hi e: 2103movhi r1, #3 10: 2107movls r1, #7 12: bf08it eq 14: 210bmoveq r1, #11 16: 4770bx lr I think before the patch there would be 'it' values before each and every condition. In fact, if you change 'bar' to, bar: cmp r0, #10 movhi r1,#3 movlo r1,#7 moveq r1,#11 bx lr You get three 'IT' conditions as 'HI' and 'LO' are not opposite. The patch seem to detect things that are the exact opposite. The 'bar' above ends up with '11' in r1 if the value is zero, but it temporarily '7'. The 2nd bar will only place 11 in r1. Fwiw, Bill Pringlemeir. Ref: https://wiki.ubuntu.com/ARM/Thumb2PortingHowto#Conditional_Execution ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 5/7] imx6: add some flexibility for defining macros
> On 12/11/2014 01:15, John Tobias wrote: >> In iMX6DQ data sheet the stack address is 0x0093FFB8 (page 383). >> While, in iMX6SDL datasheet (page 393) is 0x0091FFB8. On 12 Nov 2014, sba...@denx.de wrote: > I admit that I should take a coffe, and after that maybe I can realize > what you are saying. Anyway, it is normal for processors to set the > stack pointer. If at a certain point the stack pointer is > automatically set, well, this is another case. The internal ROM will > set the stack pointer to a well defined address when it starts, but it > does not mean that is fixed for other part of code. The problem arises > if from our code (SPL) we call functions inside the ROM, and of course > this can have conflicts if they share the same address range. This is > also not your case - if that happens, two separate stack area must be > taken. > What you are referring is the stack pointer of the ROM. Well, the > processors have different ROMs (only Freescale knows the differences) > and of course they can use differently the IRAM. But when the ROM > gives the control to the loaded image (SPL), this can use the whole > IRAM (with the use case exception I mentioned before). > That is the reason because ventana and Compulab have no problems at > all. Yes; this is my understanding as well. I think there is some HABv4 document which says what IRAM offsets are used and should be reserved fro all CPUs. For the iMx25 at least, it seems you may set the stack address to whatever you like when calling the ROM code. It is the HAB data that must not be touched. So even if calls are made to the ROM code, I think you are free to set the stack address to whatever you want. On Tue, Nov 11, 2014 at 5:02 PM, Fabio Estevam wrote: >> iMX6SDL has 128KB OCRAM, 0x0090 - 0x0091 >> iMX6DQ has 256KB OCRAM, 0x0090 - 0x0093 >> >> So, if we want 1 image to support both, we should choose 0x0091FFB8. This is true. However, you will limit the size on the iMX6DQ platform. Maybe that is fine for an SPL. I think you can also use address aliasing. Does the iMX6SDL fully decode? It maybe that 0x0093 on the iMX6SDL will go to the 0x0091 address. Did we try 0x0093FFB8 on the iMX6SDL and it doesn't work? I have used the aliasing to setup cache/non-cache ranges to the IRAM. This is nice if you have L1 (primary 1M/4M MMU section entries) and need to have non-cached entries for DMA (ethernet, usb, etc). But that is probably not relevant for the SPL? Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] imx: mx6: Add support for MX6 plugin images
On 5 Nov 2014, sba...@denx.de wrote: > Let's see the advantages you are listing here. Of course, if we are > comparing to the way was mostly used for i.MXes, it takes advantages. > Using only the DCD, most configurations must be decided at compile > time. This does not allow to have a single image for multiple boards, > for example if a different i.MX6 variant is mounted on the board. And > there are a lot of other things that are known only at runtime. > > But generally in the U-Boot project was already found solution, that > is not only valid for Freescale's i.MXes: SPL. Using SPL, this adds > much more flexibility as with plugins. You can program whatever you > want, and of course you can add any runtime detection is possible, > without intervention of the BOOT ROM. And this is not limited to ddr > settzing, clock gating or pinmux setup. > > This is not a new approach, and support for SPL was already added to > mainline - see the Gateway's ventana board. The different layout of > the pinmux is correctly handled. The processor is detected at runtime, > making possible to have a single image for several variations of the > same board. > > I see that plugins are the way found to circumvent the static > limitations of the DCD, but it is not a general solution. On the other > side, the SPL concept works well on i.MXes as on other SOCs (TIs, > Nvidia,...). This seems true that the SPL is another way to implement the 'plug-in' features as they relate to DCD. > I have not changed my mind and I still think that i.MXes in U-Boot > should not be an island in the project. Some general concepts must > work for all supported SOCs, including i.MXes. In principle, I tend to > reject this patchset, because the same new features you list here are > already available with SPL. And I encourage to use SPL for other and > new i.MX boards. I think a portion not taken care of by SPL is 2nd and subsequent image verification. The HAB ROM loader will use the 'plug-in' to initialize and load to alternate media. However, when control returns, I think that the 2nd image is authenticated. In order to do the same in the SPL, you need to restrict the IRAM locations used and make calls to the ROM code or implement some other 2nd image authentication. For non-secure boots, the SPL seems equivalent. With secondary image verification in the SPL, then I think it would be equivalent to the 'plug-in'. The SPL would be supported in all HAB versions. I don't know if the 'plug-in' is supported with earlier iMx series like the iMx2/3x series using HABv3. So an SPL with image verification seems superior, even for the iMx series by itself. Fwiw, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Relocation issue - need help!
On 23 Oct 2014, w...@denx.de wrote:
> Given that GCC 4.9.1 apparently solves this issue I wonder which
> approach we should take?
>
> Should we blacklist GCC 4.8.x (and 4.9.0) like the kernel folks are
> doing [1] ?
>
> [1] https://lkml.org/lkml/2014/10/10/272
My understanding is this is for a completely different issue. The
proposed patch is,
+# if ((GCC_VERSION >= 40800 && GCC_VERSION <= 40802) || GCC_VERSION == 40900) \
+ && defined(__arm__) && defined(CONFIG_FRAME_POINTER)
+# error Your version of gcc miscompiles stack frames
+# endif
So, the proposed patch was blacklist gcc v4.8.[012] and 4.9.0 on the
*ARM* when frame pointer tracing is enabled. This thread refers to a
different bug where 'sp' is adjusted for a return while the 'fp' will
still be references (for instance in a return calculation) above the
'sp'. It is a completely different issue than the '.data.rel.ro' which
is on all architectures.
Also, if you read further in the thread, it seems that 4.9.0 does not
exhibit this problem. The 4.9.0 was only suggested as the 'Known to
fail' of bug 58854 shows '4.9.0'.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58854
Apparently this was added during the 4.9.0 release process, but didn't
make it to the official release. In any case, unless u-boot uses
signals and/or an interrupt stack common to the mainline, this bug would
not even affect the ARM cpus with U-boot.
On 23 Oct 2014, w...@denx.de wrote:
> 4) For the problemativ 4.8.x versions of GCC, the following patch
> apparently solves the problem for my (MPC5200 based) board - guess
> this would have to be applied to all .lds files...
>diff --git a/arch/powerpc/cpu/mpc5xxx/u-boot.lds
>b/arch/powerpc/cpu/mpc5xxx/u-boot.lds
>index cd9e23f..82c86d7 100644
>--- a/arch/powerpc/cpu/mpc5xxx/u-boot.lds
>+++ b/arch/powerpc/cpu/mpc5xxx/u-boot.lds
>@@ -27,6 +27,8 @@ SECTIONS
> {
> _GOT2_TABLE_ = .;
> KEEP(*(.got2))
>+KEEP(*(.data.rel.ro))
>+KEEP(*(.data.rel.ro.local))
> KEEP(*(.got))
> PROVIDE(_GLOBAL_OFFSET_TABLE_ = . + 4);
> _FIXUP_TABLE_ = .;
> Given that GCC 4.9.1 apparently solves this issue I wonder which
> approach we should take?
>From my perspective and reading the blog below this patch seems sensible.
http://www.airs.com/blog/archives/189
I assume u-boot has no MMU enabled, then all the relocations should be
similar. This would be for all architectures though?
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
>>> On 12 September 2014 05:25, Masahiro Yamada
>>> wrote:
>>
>>>>>>> I have a qustion about lists_driver_lookup_name() function.
>>>>> On 09/14/14 21:28, Simon Glass wrote:
>>
>>>>> I would suggest still using strncmp as it is safer,
>>>>> but count also the '\0', so something like:
>>
>> On 17 Sep 2014, grinb...@compulab.co.il wrote:
>>
>>>> Why safer?
>>
>>>> Could you give me more detailed explanation?
>>
>>> On 09/17/14 11:18, Masahiro Yamada wrote:
>>
>>> Well, I'm not an expert in s/w security, but I'll try to explain...
>>
>> [snip]
>>
>>> But, again, I'm not an expert in this area, so its only a
>>> suggestion.
>>
> On 09/17/14 18:25, Bill Pringlemeir wrote:
>> I thought it was fairly apparent that the current code supports
>> passing a string that is *NOT* null terminated. This can be
>> convenient if you extract a sub-string from a command line and do not
>> need to make a copy that is NULL terminate or perform 'strtok()' type
>> magic.
On 18 Sep 2014, grinb...@compulab.co.il wrote:
> Here is the whole function:
>
> --cut--
> struct driver *lists_driver_lookup_name(const char *name)
> {
> struct driver *drv =
> ll_entry_start(struct driver, driver);
> const int n_ents = ll_entry_count(struct driver, driver);
> struct driver *entry;
> int len;
>
> if (!drv || !n_ents)
> return NULL;
>
> len = strlen(name);
>
> for (entry = drv; entry != drv + n_ents; entry++) {
> if (strncmp(name, entry->name, len))
> continue;
>
> /* Full match */
> if (len == strlen(entry->name))
> return entry;
>>
>
> /* Not found */
> return NULL;
>>
> --cut--
>
> and... no, the code does not support passing a string that is
> not null terminated.
Then using the strncmp() seems useless for security reasons? The 'len'
is not passed in by the caller and 'strlen()' will have the same
problems that 'strcmp()' would for read buffer overflows? I would guess
the code was cribbed from where 'len' was passed? In that case, it
would support strings that are not null terminated.
Sorry, I didn't look and understand your query now.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
> On 12 September 2014 05:25, Masahiro Yamada wrote:
>>>>> I have a qustion about lists_driver_lookup_name() function.
>>>>> for (entry = drv; entry != drv + n_ents; entry++) {
>>>>> if (strncmp(name, entry->name, len))
>>>>> continue;
>>>>> /* Full match */
>>>>> if (len == strlen(entry->name))
>>>>> return entry;
>>>>> }
>>> On 09/14/14 21:28, Simon Glass wrote:
>>> I would suggest still using strncmp as it is safer,
>>> but count also the '\0', so something like:
On 17 Sep 2014, grinb...@compulab.co.il wrote:
>> Why safer?
>> Could you give me more detailed explanation?
> On 09/17/14 11:18, Masahiro Yamada wrote:
> Well, I'm not an expert in s/w security, but I'll try to explain...
[snip]
> But, again, I'm not an expert in this area, so its only a suggestion.
I thought it was fairly apparent that the current code supports passing
a string that is *NOT* null terminated. This can be convenient if you
extract a sub-string from a command line and do not need to make a copy
that is NULL terminate or perform 'strtok()' type magic.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 1/2] mtd: nand: add Freescale vf610_nfc driver
On 11 Sep 2014, ste...@agner.ch wrote: > This adds initial support for Freescale NFC (NAND Flash Controller) > found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. > The driver is called vf610_nfc since this is the first supported > and tested hardware platform supported by the driver. > > Signed-off-by: Stefan Agner Acked-by: Bill Pringlemeir ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3 3/4] mtd: nand: add Freescale vf610_nfc driver
>>>>> On 14 Aug 2014, ste...@agner.ch wrote:
>>>>> This adds initial support for Freescale NFC (NAND Flash
>>>>> Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125
>>>>> and others. However, this driver is only tested on Vybrid.
>>> On Wed, 2014-08-13 at 22:32, Scott Wood wrote:
>>>> raw_writel() is itself something that should only be used for
>>>> hand-optimized sections. For non-performance-critical code you
>>>> should use normal writel() so that you don't need to worry about
>>>> manually adding I/O barriers.
>>> Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
[regarding memcpy() in the driver]
>>>> Maybe a comment is fine? It seems the Vybrid is safe for
>>>> different access sizes, but it is possible that some other CPU
>>>> might not be able to access this memory via 32/16/8 bit accesses
>>>> and 'memcpy()' may not be appropriate. It seems that 'natural'
>>>> size of the NFC controller itself is 32bits and the CPU interface
>>>> does lane masking. Ie, boot mode documentation talks about
>>>> remapping 'sram_physical_addr[13:3] =
>>>> {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used
>>>> (hopefully one based numbers). This is just my guess...
>> On 18 Aug 2014, ste...@agner.ch wrote:
>>> What assumptions do you make how memcpy accesses memory? This latest
>>> patch now uses the optimized versions from the kernel... Maybe they
>>> even try to access 64-bit width (the NIC interconnect supports
>>> 64-bit access)
[snip]
> Am 2014-08-18 18:38, schrieb Bill Pringlemeir:
>> My only point is that the SRAM buffers use the same interface as the
>> main Nand controller register banks. So using 'readl/writel' for the
>> register, but not the SRAM buffers seems inconsistent.
>> So to address this inconsistency, I was thinking that we should at
>> least have a comment?
On 19 Aug 2014, ste...@agner.ch wrote:
> IMHO, we just treat this as if its memory and I guess this is fine for
> a buffer. memcpy knows how to copy data, and takes care if the
> architecture needs aligned access when reading 32-bit width, or
> similar requirements. We do not know whether memcpy really uses 32-bit
> accesses, hence this comment might even be wrong. In a short test, I
> could also access the buffer in byte/word length (tested using
> md.b/md.w).
> Also, I assume this just works for a different architecture too. If
> not, the one using this driver the first time on a different
> architecture would see this pretty quickly I guess :-)
[snip]
> In our case, a barrier just after the memcpy would be sufficient.
I would suggest you make a 'vf610_nfc_memcpy()' [or even from/to
variants if you are pendantic] which can be a wrapper function of just
'memcpy'. Just the like the readl/writel wrappers this will collect the
BUS accesses into one place. So they are documented for people porting
the code. Trying to accommodate some future insane hardware hookup seems
futile beyond this?
Otherwise, I will add an 'Ack' or 'Reviewed-By' from me if you like. I
am sorry, I don't know what if anything is appropriate.
Thanks,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/4] mtd: nand: add Freescale NFC driver
On 18 Aug 2014, ste...@agner.ch wrote:
> Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
>>> On 14 Aug 2014, ste...@agner.ch wrote:
>>>
>>> This adds initial support for Freescale NFC (NAND Flash Controller)
>>> found in ARM Vybrid SoC's, Power Architecture MPC5125 and others.
>>> However, this driver is only tested on Vybrid.
>>
>> This is only to expand on the nand controller register and SRAM use.
>>
>> [snip]
>>
>>> diff --git a/drivers/mtd/nand/vf610_nfc.c
>>> b/drivers/mtd/nand/vf610_nfc.c new file mode 100644 index
>>> 000..3150ac1 --- /dev/null +++ b/drivers/mtd/nand/vf610_nfc.c
>>
>> [snip]
>>
>>> +static inline u32 vf610_nfc_read(struct mtd_info *mtd, uint reg) +{
>> Ok, we always use readl/writel. This is fine, but a little slower
>> and bigger. I may try a register cache if I resubmit to the Linux
>> MTD as per Scott's suggestion. Especially, this version is good for
>> an incremental patch.
> I measured the difference and get 1MB/s
> Full pages, readl/writel:
> NAND read: device 0 offset 0x20, size 0x80
> 8388608 bytes read in 772 ms (10.4 MiB/s): OK
> Full pages, __raw_readl/__raw_writel
> NAND read: device 0 offset 0x20, size 0x80
> 8388608 bytes read in 696 ms (11.5 MiB/s): OK
> Ok, this is actually quite a lot. Especially since I already optimized
> the C code (by not using the helper functions like nfc_set/nfc_clear
> in vf610_nfc_send_command), one would think there is now almost no
> optimization potential. I looked into the disassembled code and could
> narrow down the issue. Due to the memory barriers, all offsets were
> calculated on each register access (nfc base to reg base, and add reg
> offset), multiple instances of:
> 20: e59cc120ldr ip, [ip, #288] ; 0x120
> 24: e59cc134ldr ip, [ip, #308] ; 0x134
> I optimized the code again and calculate the offsets manually and
> access __raw_readl/__raw_writel rather then vf610_nfc_read/write in
> the vf610_nfc_send_command(s) function, I get the full speed again:
> NAND read: device 0 offset 0x20, size 0x80
> 8388608 bytes read in 687 ms (11.6 MiB/s): OK
I think what you have is fine. The 10BM/s versus 11MB/s is not
insignificant, but it is not a huge difference. I expect that the
driver is already better than the others; especially the Imx-25 was only
7.7MB/s read and 4.6Mb/s write from Linux mtd tests. Although the end
user might like to wait 10% less for an image to load.
Also, probably a lot of people care about code size. This is not so
much a case for U-Boot as it is usually machine specific and doesn't
support several SOCs afaik.
However, the more specific you make the optimization to the platform,
the less likely it is to extend well. We also wish to have this work
well with different gcc versions and CPUs (PowerPC, etc). The
'readl/writel' handicap the compiler. Although they are more likely to
work with a wide variety of buses.
[snip]
> On Wed, 2014-08-13 at 22:32, Scott Wood wrote:
>> raw_writel() is itself something that should only be used for
>> hand-optimized sections. For non-performance-critical code you
>> should use normal writel() so that you don't need to worry about
>> manually adding I/O barriers.
>
> The reason I choosed readl/writel instead of the raw variants is to
> preserve align with other drivers...
>> care. Maybe a comment is fine? It seems the Vybrid is safe for
>> different access sizes, but it is possible that some other CPU might
>> not be able to access this memory via 32/16/8 bit accesses and
>> 'memcpy()' may not be appropriate. It seems that 'natural' size of
>> the NFC controller itself is 32bits and the CPU interface does lane
>> masking. Ie, boot mode documentation talks about remapping
>> 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying
>> that bits 2,1 are not used (hopefully one based numbers). This is
>> just my guess...
> What assumptions do you make how memcpy accesses memory? This latest
> patch now uses the optimized versions from the kernel... Maybe they
> even try to access 64-bit width (the NIC interconnect supports 64-bit
> access)
The memcpy() itself could use anything. 64bits is possible on AXI/NIC.
The 'PBRIDGE' is 64bit, but I think the AIPS/IPS (apparently AIPS means
'AHB-lite to IPS) are 32bit. At least that is the case on the Imx25
which has a different AIPS version. I assumed the 'memcpy()' was using
32bits but this certainly isn't explicit in the code.
The majority of the register banks are non-volatile with this
controller. Instead of running multiple NAND progra
Re: [U-Boot] [PATCH v2 3/4] mtd: nand: add Freescale NFC driver
are the same 'bus' interface as the registers. We should
be just as worried about this SRAM buffer memory as the memory mapped
registers, shouldn't we? Is a barrier() before reading and a barrier()
after writing fine for U-Boot? Personally, I think they are safe as
only the 'vf610_nfc_set(mtd, NFC_FLASH_CMD2, START_BIT)' needs some
care. Maybe a comment is fine? It seems the Vybrid is safe for
different access sizes, but it is possible that some other CPU might not
be able to access this memory via 32/16/8 bit accesses and 'memcpy()'
may not be appropriate. It seems that 'natural' size of the NFC
controller itself is 32bits and the CPU interface does lane masking.
Ie, boot mode documentation talks about remapping
'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying
that bits 2,1 are not used (hopefully one based numbers). This is just
my guess...
The VF6xx page has a documentation tab,
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=VF6xx
There is an app note, AN4947 'Understanding Vybrid Architecture', which
describes some timing details for the AHB bus (where this flash
controller is connected). Pg21 Table 7 of that document gives some
measurements. The QSPI is a similar peripheral on the AHB. The first
and second lines give accesses of 4408 and subsequent accesses are 2770
Cortex-A5 clocks. Normal SDRAM is 258 and 8 clocks. Ie, it is quite
important in places to minimize accesses and try to make them
sequential.
However, it looks like most U-Boot NAND drivers use the memcpy()? With
the exceptions of fsl_elbc_nand.c, fsl_ifc_nand.c, and mpc5121_nfc.c.
Maybe it doesn't matter...
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/4] mtd: nand: add Freescale NFC driver
On 14 Aug 2014, ste...@agner.ch wrote:
> This adds initial support for Freescale NFC (NAND Flash Controller)
> found in ARM Vybrid SoC's, Power Architecture MPC5125 and others.
> However, this driver is only tested on Vybrid.
>
> Signed-off-by: Stefan Agner
> ---
> drivers/mtd/nand/Makefile| 1 +
> drivers/mtd/nand/vf610_nfc.c | 706
> ++> +
> 2 files changed, 707 insertions(+)
> create mode 100644 drivers/mtd/nand/vf610_nfc.c
>
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> new file mode 100644
> index 000..3150ac1
> --- /dev/null
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -0,0 +1,706 @@
> +/*
> + * Copyright 2009-2014 Freescale Semiconductor, Inc. and others
> + *
[snip]
> +/* Count the number of 0's in buff upto max_bits */
> +static inline int count_written_bits(uint8_t *buff, int size, int max_bits)
> +{
> + uint32_t *buff32 = (uint32_t *)buff;
> + int k, written_bits = 0;
> +
> + for (k = 0; k < (size / 4); k++) {
> + written_bits += hweight32(~buff32[k]);
> + if (written_bits > max_bits)
> + break;
> + }
> +
> + return written_bits;
> +}
That is a nice change.
> +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, u_char *dat)
> +{
> + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> + u8 ecc_status;
> + u8 ecc_count;
> + int flip;
> +
> + ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
> + ecc_count = ecc_status & ECC_ERR_COUNT;
> + if (!(ecc_status & ECC_STATUS_MASK))
> + return ecc_count;
> +
> + /* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
> + flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> +
> + /* ECC failed. */
> + if (flip > ecc_count)
> + return -1;
Sorry, I missed this in version one of the patch. The original had,
< if (flip > ecc_count) {
< nfc->page = -1;
---
> if (flip > ecc_count)
522d508
< }
I can see why you removed this (nfc->page = -1). However, I think that
higher layers may want to re-read on an error in case of un-stable bits?
It is very little code to ensure a re-read in case of ECC failure. The
2nd physical read may pass whereas the first failed. This path is rare,
but maybe important? A higher layer may migrate the data in this case;
just as with a corrected bits. But maybe U-Boot will never do this?
> +
> + /* Erased page. */
> + memset(dat, 0xff, nfc->chip.ecc.size);
> + return 0;
> +}
Regards,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
On 13 Aug 2014, scottw...@freescale.com wrote: > On Wed, 2014-08-13 at 17:44 -0400, Bill Pringlemeir wrote: >> Regarding "can't know in advance", I think that some of the register >> values maybe set by the boot rom. This might make more sense for >> Linux than U-Boot. However, after the initial configuration, many do >> need the 'read/modify/write' as there are many bit fields with >> different functionality. > If the register is only modified by software, and not asynchronously > by hardware, then you could read the value once when the driver > starts, and cache its value to avoid a reportedly expensive I/O access > every time you need to modify it. Yes, that is a good point. The regmap interface could be used in the Linux kernel. I don't see any infrastructure like that in U-Boot? It is fairly simple to re-invent as this driver only needs the flat structure. Regards, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
On 13 Aug 2014, scottw...@freescale.com wrote: > On Tue, 2014-08-12 at 18:58 -0400, Bill Pringlemeir wrote: >> On 12 Aug 2014, scottw...@freescale.com wrote: >> >>> On Tue, 2014-08-12 at 23:13 +0200, Stefan Agner wrote: >>>> Am 2014-08-12 00:33, schrieb Scott Wood: >>>>> You should always be using raw I/O accessors. If the intent is to >>>>> bypass I/O ordering for certain registers, the raw accessors >>>>> should already be skipping that. >> >>>> Ok, will do. >> >>> Sorry, the above should say "always be using I/O accesors", not >>> always raw ones. >> >> This is probably because of me. There are lines like this for >> configuration, >> >> /* Set configuration register. */ >> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT); >> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BUFNO_AUTO_INCR_BIT); >> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BOOT_MODE_BIT); >> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT); >> nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT); >> >> If you use some sort of 'volatile', you are saying to the compiler >> that these lines are *not*, >> >> ldr r0, [r1, #CONFIG]# read previous value >> ldr r2, =~MASK >> orr r0, #FAST_FLASH_BIT # set one bit. >> and r0, r2 # clear all bits at once. >> str r0, [r1, #CONFIG]# write it back. >> >> Instead it will change into five different load/modify/stores. The >> memory itself is just like SRAM except a few registers that are >> actually volatile; ie changed via the hardware. > If this is performance-critical then it would be best to modify the > code to explicitly do one read from I/O (if you can't know in advance > what the existing value will be), clear all the bits you're going to > clear, then have one explicit write back to the I/O device -- rather > than treating it as ordinary memory for the compiler to optimize > accesses to. > If nothing else, it makes the code clearer to make I/O accesses > explicit, and reduces the likelihood that people see I/O accesses > without accessors and go on to do the same in some other less safe > circumstance. Yes, absolutely. The issue is that the 'nfc_clear()', etc seems more programmer friendly to me. It was from an original driver. I believe this is what Stefan means by 'optimized' and not 'raw_readl/raw_writel' versus 'readl/writel'. I believe that this is smaller/faster in all cases; not just hot paths as the code should be smaller. I tried to modify the accessor to leave the original code as is. The registers are a bunch of bit fields. It is clear to read them each as being set/cleared on a different lines? However, the whole group can be set at once at the machine level. Sometimes the bit fields aren't so related. So, if you are trying to write the code about what is happening to the NAND flash (Ie cycles to run) then ganging the NFC controller registers together can make things a little more obscure. Maybe this is a small price to pay if the register ordering is more of an issue to people. The accesses are generally all order independent *when idle*. There is one bit of the NFC_FLASH_CMD2 register as bit0 or START_BIT in the code. When it is toggled, the NFC controller starts it's business and then only a few register can be polled or an interrupt generated. In this phase, no register changes should take place or at least care should be taken. I have only compiler barriers in the driver I submitted to the Linux MTD. It is possible that the PowerPC or other multi-issue CPUs might need the iowmb() or otherwise when the 'START_BIT' is toggled. I had thought of this in the mean time, so thank-you for bringing it up. Allowing the compiler to re-order the settings of the registers when idle can let it decide to do all the zeroing at once, etc depending on what is optimal. The 'read/modify many/write' is a happy medium for me. Minimizing accesses to the registers is important as these often involve a slow bus interface and also generate a lot of code for load/store CPUs. Regarding "can't know in advance", I think that some of the register values maybe set by the boot rom. This might make more sense for Linux than U-Boot. However, after the initial configuration, many do need the 'read/modify/write' as there are many bit fields with different functionality. Thanks, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
On 13 Aug 2014, ste...@agner.ch wrote: > Funny is, the size is bigger in the first uninlined case... Maybe GCC > inlined the function only for some calls, I did not checked that... > > With if/else > text data bss dec hex filename > 2395 2904 0529914b3 drivers/mtd/nand/fsl_nfc.o This is totally sensible. In some cases, the function epilogue and prologue are actually bigger than the function body. Also, the implicit call means that the caller must save some temporary registers (R0-R3). For the simple nfc_read() and nfc_write(), I would expect the size to get bigger if they are not inlined. Especially, gcc can recognize that the same memory location is being operated on and collapse the accesses. Anyways, thanks for showing that the previous code was depending too much on compiler knowledge. Your current plan sounds promising. Regards, Bill Pringlemeir. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
On 13 Aug 2014, ste...@agner.ch wrote:
> Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
> [snip]
>>>>>> +static u32 nfc_read(struct mtd_info *mtd, uint reg)
>>>>>> +{
>>>>>> +struct fsl_nfc *nfc = mtd_to_nfc(mtd);
>>>>>> +
>>>>>> +if (reg == NFC_FLASH_STATUS1 ||
>>>>>> +reg == NFC_FLASH_STATUS2 ||
>>>>>> +reg == NFC_IRQ_STATUS)
>>>>>> +return __raw_readl(nfc->regs + reg);
>>>>>> +/* Gang read/writes together for most registers. */
>>>>>> +else
>>>>>> +return *(u32 *)(nfc->regs + reg);
>>>>>> +}
>>>>>> +
>>>>>> +static void nfc_write(struct mtd_info *mtd, uint reg, u32 val)
>>>>>> +{
>>>>>> +struct fsl_nfc *nfc = mtd_to_nfc(mtd);
>>>>>> +
>>>>>> +if (reg == NFC_FLASH_STATUS1 ||
>>>>>> +reg == NFC_FLASH_STATUS2 ||
>>>>>> +reg == NFC_IRQ_STATUS)
>>>>>> +__raw_writel(val, nfc->regs + reg);
>>>>>> +/* Gang read/writes together for most registers. */
>>>>>> +else
>>>>>> +*(u32 *)(nfc->regs + reg) = val;
>>>>>> +}
>>>>>
>>>>> You should always be using raw I/O accessors. If the intent is to
>>>>> bypass I/O ordering for certain registers, the raw accessors
>>>>> should already be skipping that.
>>
>>>> Ok, will do.
>>
>>> Sorry, the above should say "always be using I/O accesors", not
>>> always raw ones.
>>
>> This is probably because of me. There are lines like this for
>> configuration,
>>
>> /* Set configuration register. */
>> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT);
>> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BUFNO_AUTO_INCR_BIT);
>> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BOOT_MODE_BIT);
>> nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT);
>> nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT);
>>
>> If you use some sort of 'volatile', you are saying to the compiler
>> that these lines are *not*,
>>
>> ldr r0, [r1, #CONFIG]# read previous value
>> ldr r2, =~MASK
>> orr r0, #FAST_FLASH_BIT # set one bit.
>> and r0, r2 # clear all bits at once.
>> str r0, [r1, #CONFIG]# write it back.
>>
>> Instead it will change into five different load/modify/stores. The
>> memory itself is just like SRAM except a few registers that are
>> actually volatile; ie changed via the hardware.
>>
>> Particularly bad are the memcpy_io() on the ARM. Using these results
>> in about 1/2 to 1/4 of the performance of the drivers through-put on
>> the Vybrid. I can see that using accessors is good, but just
>> replacing this with 'writel' will result in significantly more code
>> and slower throughput.
>>
>> If people insist on this, then something like,
>>
>> val = nfc_read(mtd, NFC_REG);
>> val = nfc_clear(val, CONFIG_ADDR_AUTO_INCR_BIT);
>> val = nfc_clear(val, CONFIG_BUFNO_AUTO_INCR_BIT);
>> val = nfc_clear(val, CONFIG_BOOT_MODE_BIT);
>> val = nfc_clear(val, CONFIG_DMA_REQ_BIT);
>> val = nfc_set(val, CONFIG_FAST_FLASH_BIT);
>> nfc_write(mtd, NFC_REG, val);
>>
>> would result in the same code with 'nfc_read()' and 'nfc_write()'
>> using the I/O accessors. I found this more difficult to read for the
>> higher level functions. Instead some some standard macros for
>> setting and clearing bits could be used. The original driver was
>> using 'nfc_set()' and 'nfc_clear()'. Maybe they should just go.
>>
>
> I just applied this change:
>
> diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
> index df2c8be..01c010f 100644
> --- a/drivers/mtd/nand/fsl_nfc.c
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -191,26 +191,14 @@ static u32 nfc_read(struct mtd_info *mtd, uint
> reg)
> {
> struct fsl_nfc *nfc = mtd_to_nfc(mtd);
>
> - if (reg == NFC_FLASH_STATUS1 ||
> - reg == NFC_FLASH_STATUS2 ||
> - reg == NFC_IRQ_STATUS)
> - return __raw_readl(nfc->regs + reg);
> - /* Gang read/writes together for most registers. */
> - else
> - return *(u32 *)(nfc->regs + reg);
> + return __raw_r
Re: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
On 12 Aug 2014, scottw...@freescale.com wrote:
> On Tue, 2014-08-12 at 23:13 +0200, Stefan Agner wrote:
>> Am 2014-08-12 00:33, schrieb Scott Wood:
>>> On Wed, 2014-08-06 at 10:59 +0200, Stefan Agner wrote:
This adds initial support for Freescale NFC (NAND Flash
Controller). The IP is used in ARM based Vybrid SoCs as well as on
some PowerPC devices. This driver is only tested on Vybrid.
Signed-off-by: Stefan Agner
---
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/fsl_nfc.c | 676
+
2 files changed, 677 insertions(+)
create mode 100644 drivers/mtd/nand/fsl_nfc.c
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index bf1312a..85cb0dd 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_NAND_KB9202) += kb9202_nand.o
obj-$(CONFIG_NAND_KIRKWOOD) += kirkwood_nand.o
obj-$(CONFIG_NAND_KMETER1) += kmeter1_nand.o
obj-$(CONFIG_NAND_MPC5121_NFC) += mpc5121_nfc.o
+obj-$(CONFIG_NAND_FSL_NFC) += fsl_nfc.o
obj-$(CONFIG_NAND_MXC) += mxc_nand.o
>>>
>>> Could you explain how this differs from mpc5121_nfc, mxc_nand, etc?
>>> If it's truly different enough to deserve its own driver, it should
>>> at least get a more specific name.
>>>
>>
>> I'm not really an expert in NAND devices, but from looking into the
>> reference manuals and drivers, I summarize those differences:
>> mxc_nand: Supports 3 NAND Flash controller found in i.MX ARM SoCs:
>> V1: MX31/MX27, 16 Bit Registers
>> V2.1: MX25/MX35, 16 Bit Registers,
>> V3.2: MX51/MX53, 32 Bit Registers, 12 address registers...
>> All three have no DMA controller integrated.
>> mpc5121_nfc: Supports the MPC5121 Power Architecture Processor NAND
>> flash controller. Big Endian, but other than this, this IP looks very
>> similar to the V2.1 NFC above (looking at the registers and the block
>> diagram).
> Yes, this is the similarity that prompted me to ask the question... I
> asked it back when those drivers were submitted, but was unable to get
> the submitter of each to work together on a common driver.
I am familiar with the mxc_nand on the imx. The register set might look
similar (ie, the register names) but when you look in depth at the bits
and their function, it is pretty clear it is different. Some people
inside Freescale have said that this is a completely different IP.
>> fsl_nfc: Supports VF610 (2011), however should be easily portable to
>> MPC5125 Power Architecture Processor (2009) and MCF5441X ColdFire V4
>> Microprocessor (2009)
>> All three share the almost identical Register Layout, similar block
>> diagram and have integrated DMA controller.
>>
>> IMHO, this IP really deserves a own driver.
>>
>> Also, from my humble perspective, it might have made more sense to
>> integrate mpc5121_nfc into mxc_nand. In return, splitting out
>> mxc_nand NFC V3.2 (looking at the ifdefs and quite different register
>> layout). Anyway, not part of the topic here..
>>
>> Regarding naming: A more specific name makes sense since Freescale
>> calls all its Flash Controllers "NFC". I suggest vf610_nfc because
>> that SoC is really is making use of the driver today... Looking at
>> release dates, mpc5125_nfc would make sense as well.
> OK.
Here we talked about the name,
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/251698.html
It is also present on a Kinetis K70 chip.
What should be important is that we use the same name in both U-boot
and Linux? Won't it be confusing to call it something different? I
really don't care what it is called as long as it is consistent.
+static u32 nfc_read(struct mtd_info *mtd, uint reg)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+ if (reg == NFC_FLASH_STATUS1 ||
+ reg == NFC_FLASH_STATUS2 ||
+ reg == NFC_IRQ_STATUS)
+ return __raw_readl(nfc->regs + reg);
+ /* Gang read/writes together for most registers. */
+ else
+ return *(u32 *)(nfc->regs + reg);
+}
+
+static void nfc_write(struct mtd_info *mtd, uint reg, u32 val)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+ if (reg == NFC_FLASH_STATUS1 ||
+ reg == NFC_FLASH_STATUS2 ||
+ reg == NFC_IRQ_STATUS)
+ __raw_writel(val, nfc->regs + reg);
+ /* Gang read/writes together for most registers. */
+ else
+ *(u32 *)(nfc->regs + reg) = val;
+}
>>>
>>> You should always be using raw I/O accessors. If the intent is to
>>> bypass I/O ordering for certain registers, the raw accessors should
>>> already be skipping that.
>> Ok, will do.
> Sorry, the above should say "always be using I/O accesors", not always
> raw ones.
This is probably because of me. There are lines like this for
configuration,
/* Set configuration register. */
nfc_clear(mtd, NFC_FLASH
Re: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
On 6 Aug 2014, ste...@agner.ch wrote:
> This adds initial support for Freescale NFC (NAND Flash Controller).
> The IP is used in ARM based Vybrid SoCs as well as on some PowerPC
> devices. This driver is only tested on Vybrid.
>
> Signed-off-by: Stefan Agner
> ---
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/fsl_nfc.c | 676
> > +
> 2 files changed, 677 insertions(+)
> create mode 100644 drivers/mtd/nand/fsl_nfc.c
> diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
> new file mode 100644
> index 000..df2c8be
> --- /dev/null
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -0,0 +1,676 @@
[snip]
> +#define NFC_TIMEOUT (1000)
> +
> +/* ECC status placed at end of buffers. */
> +#define ECC_SRAM_ADDR((PAGE_2K+256-8) >> 3)
> +#define ECC_STATUS_MASK 0x80
> +#define ECC_ERR_COUNT0x3F
> +
> +struct fsl_nfc {
> + struct mtd_info *mtd;
> + struct nand_chip chip;
> + struct device *dev;
> + void __iomem *regs;
> + //wait_queue_head_t irq_waitq;
I think u-boot doesn't like C++ comments?
> + uint column;
> + intspareonly;
> + intpage;
> + /* Status and ID are in alternate locations. */
> + intalt_buf;
> +#define ALT_BUF_ID 1
> +#define ALT_BUF_STAT 2
> + struct clk*clk;
> +};
> +
> +#define mtd_to_nfc(_mtd) (struct fsl_nfc *)((struct nand_chip
> *)_mtd->priv)->priv;
[snip]
> +static int nfc_correct_data(struct mtd_info *mtd, u_char *dat,
> + u_char *read_ecc, u_char *calc_ecc)
> +{
> + struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> + u32 ecc_status;
> + u8 ecc_count;
> + int flip;
> +
> + /*
Some extra ws here Ie, '/* ' with a space after the star. Maybe that
is from me?
> + * Errata: ECC status is stored at NFC_CFG[ECCADD] +4 for
> + * little-endian and +7 for big-endian SOC. Access as 32 bits
> + * and use low byte.
> + */
This appears to be documented in the latest Vybrid manual (Rev7).
> + ecc_status = __raw_readl(nfc->regs + ECC_SRAM_ADDR * 8 + 4);
> + ecc_count = ecc_status & ECC_ERR_COUNT;
> + if (!(ecc_status & ECC_STATUS_MASK))
> + return ecc_count;
[snip]
> +static void nfc_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +}
> +
> +struct nfc_config {
> + int hardware_ecc;
> + int width;
> + int flash_bbt;
> +};
> +
> +//static int nfc_probe(struct platform_device *pdev)
Another C++ comment.
All minor nits. I am certainly ok with this code being included in
u-boot.
Regards,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
