Re: [U-Boot] [PATCH] Fix logic for selection of CONFIG_SYS_DEF_EEPROM_ADDR
Any comments on this patch? If not, could it please be applied/merged? It fixes a definite bug on the HWW-1U-1A board. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ On Dec 15, 2011, at 21:15, Kyle Moffett wrote: > A board with CONFIG_SPI and CONFIG_ENV_EEPROM_IS_ON_I2C will get: > #define CONFIG_SYS_DEF_EEPROM_ADDR 0 > > Instead of the expected: > #define CONFIG_SYS_DEF_EEPROM_ADDR CONFIG_SYS_I2C_EEPROM_ADDR > > As a result, the "eeprom" command is unusable because it calls > i2c_read() and i2c_write() but always uses an address of 0x00. > > This fixes the logic for that particular case, hopefully without > breaking any other board configurations. > > Signed-off-by: Kyle Moffett > Cc: Heiko Schocher > --- > include/common.h |6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/common.h b/include/common.h > index 5cfdd76..8a1b401 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -402,13 +402,13 @@ extern void pic_write (uchar reg, uchar val); > * Set this up regardless of board > * type, to prevent errors. > */ > -#if defined(CONFIG_SPI) || !defined(CONFIG_SYS_I2C_EEPROM_ADDR) > +#if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) > +# define CONFIG_SYS_DEF_EEPROM_ADDR 0 > +#elif !defined(CONFIG_SYS_I2C_EEPROM_ADDR) > # define CONFIG_SYS_DEF_EEPROM_ADDR 0 > #else > -#if !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) > # define CONFIG_SYS_DEF_EEPROM_ADDR CONFIG_SYS_I2C_EEPROM_ADDR > #endif > -#endif /* CONFIG_SPI || !defined(CONFIG_SYS_I2C_EEPROM_ADDR) */ > > #if defined(CONFIG_SPI) > extern void spi_init_f (void); > -- > 1.7.7.3 > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] USB: add CONFIG_USB_INIT to autoinitialize USB before main_loop
-- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ >>> This allows systems to pause autoboot with USB keyboard. Tested on >>> tegra2 seaboard. >>> >>> Signed-off-by: Allen Martin >> >> Can't you just add "usb reset" to preboot env? > > The point is to be able to use USB keyboard to stop autoboot on systems > where USB keyboard is the primary (or only) input device. This needs > to happen before BOOTCOMMAND gets parsed. I think the "preboot" environment variable already *is* run before starting the autoboot timer? Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/3] usb_storage: Fix EHCI "out of buffer pointers" with CD-ROM
On Dec 20, 2011, at 13:20, Mike Frysinger wrote: > On Tuesday 20 December 2011 12:41:14 Kyle Moffett wrote: >> +/* >> + * The U-Boot EHCI driver cannot handle more than 4096*5 bytes in a >> + * transfer without running itself out of qt_buffers. >> + */ >> +ss->max_xfer_blk = (4096*5)/dev_desc->blksz; > > spaces around those operators Fixed, thanks for the catch! Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] fs/fat: Fix FAT detection to support non-DOS partition tables
On Dec 20, 2011, at 13:20, Mike Frysinger wrote:
> On Tuesday 20 December 2011 12:41:12 Kyle Moffett wrote:
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>>
>> +static disk_partition_t cur_part_info = {
>> +.start = 0,
>> +.size = 0,
>> +.blksz = 512,
>> +.name = "",
>> +.type = "",
>> +};
>
> there any way we could delay that initialization of blksz to runtime ? if
> that wasn't there, cur_part_info would be in .bss instead of .data.
I redid this patch several times before I figured out what was going
on and I ended up with both paths fully initializing this struct, so
there's really no need for an initializer at all.
I'll remove it for the next round.
Thanks for the review!
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] drivers/net/e1000.c: Fix GCC 4.6 build warnings
On Dec 20, 2011, at 12:36, Anatolij Gustschin wrote: > Fix: > e1000.c: In function 'e1000_read_mac_addr': > e1000.c:1149:2: warning: dereferencing type-punned pointer > will break strict-aliasing rules [-Wstrict-aliasing] > > e1000.c:1149:2: warning: dereferencing type-punned pointer > will break strict-aliasing rules [-Wstrict-aliasing] > > Signed-off-by: Anatolij Gustschin > Cc: Kyle Moffett Looks great, thanks! Acked-by: Kyle Moffett Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] drivers/net/e1000_spi.c: Fix build warnings
On Dec 20, 2011, at 07:29, Anatolij Gustschin wrote:
> Fix:
> e1000_spi.c: In function 'spi_free_slave':
> e1000_spi.c:115: warning: unused variable 'hw'
> e1000_spi.c: In function 'do_e1000_spi':
> e1000_spi.c:472: warning: 'checksum' may be used uninitialized in this
> function
> e1000_spi.c:472: note: 'checksum' was declared here
Acked-by: Kyle Moffett
This is great, thanks!
I actually thought that the "checksum" fix had already made it
into Wolfgang's tree, but I can't find it now that I'm looking
for it.
The really frustrating thing is that on my test system I have
seen the "unused variable" warning for a while now (although I
was not sure what to do about it), but despite the fact that
the "checksum" variable is very clearly improperly initialized
I don't get that warning out of my compiler.
Oh, right, I'm using GCC 4.4 right now and it needs 4.6+
Ironically enough, I have never had the checksum computation
produce an incorrect result, Linux always thinks the result is
correct. It must always get a zero in that register somehow.
Cheers,
Kyle Moffett
> Signed-off-by: Anatolij Gustschin
> Cc: Kyle Moffett
> ---
> drivers/net/e1000_spi.c |5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/e1000_spi.c b/drivers/net/e1000_spi.c
> index 5491780..5f774f4 100644
> --- a/drivers/net/e1000_spi.c
> +++ b/drivers/net/e1000_spi.c
> @@ -1,4 +1,5 @@
> #include "e1000.h"
> +#include
>
> /*---
> * SPI transfer
> @@ -112,7 +113,7 @@ struct spi_slave *spi_setup_slave(unsigned int bus,
> unsigned int cs,
>
> void spi_free_slave(struct spi_slave *spi)
> {
> - struct e1000_hw *hw = e1000_hw_from_spi(spi);
> + __maybe_unused struct e1000_hw *hw = e1000_hw_from_spi(spi);
> E1000_DBG(hw->nic, "EEPROM SPI access released\n");
> }
>
> @@ -469,7 +470,7 @@ static int do_e1000_spi_program(cmd_tbl_t *cmdtp, struct
> e1000_hw *hw,
> static int do_e1000_spi_checksum(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
> int argc, char * const argv[])
> {
> - uint16_t i, length, checksum, checksum_reg;
> + uint16_t i, length, checksum = 0, checksum_reg;
> uint16_t *buffer;
> boolean_t upd;
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] drivers/net/e1000.c: Fix GCC 4.6 build warnings
On Dec 20, 2011, at 10:49, Anatolij Gustschin wrote:
> Fix:
> e1000.c: In function 'e1000_read_mac_addr':
> e1000.c:1149:2: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
> e1000.c:1149:2: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
[...]
> #ifdef CONFIG_E1000_FALLBACK_MAC
> - if ( *(u32*)(nic->enetaddr) == 0 || *(u32*)(nic->enetaddr) == ~0 ) {
> + if (get_unaligned_be32(nic->enetaddr) == 0 ||
> + get_unaligned_be32(nic->enetaddr) == ~0) {
> unsigned char fb_mac[NODE_ADDRESS_SIZE] =
> CONFIG_E1000_FALLBACK_MAC;
>
> memcpy (nic->enetaddr, fb_mac, NODE_ADDRESS_SIZE);
No, if you are going to fix this code then make it use the right
function for the job: is_valid_ether_addr()
Furthermore, while the E1000 chipset does not generally work very
well without a proper SPI EEPROM image (if at all), I think it
would be better for the driver to load successfully and use the
"macaddr" from the U-Boot environment instead of some hardcoded
compile-time constant.
IE: That whole code block should be ripped out and instead just
tweak the "valid MAC address" check further down.
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Cannot access a FAT filesystem in an El-Torito partition
On Dec 19, 2011, at 15:21, Kyle Moffett wrote: > The U-Boot FAT driver appears to manually check for the existence of > an MS-DOS partition table, even when CONFIG_DOS_PARTITION is present > and working. > > As a result, it is not possible to use the FAT driver on an ISO9660 > El-Torito boot volume, because it does not have a DOS MBR and does > not pass the magic number check. > > It looks like the code in the FAT driver to check DOS MBRs is just > legacy code from before libpart existed, and I would like to remove > it, except several board configs seem to set CONFIG_CMD_FAT but do > not set CONFIG_DOS_PARTITION: > include/configs/dbau1x00.h > include/configs/mv88f6281gtw_ge.h > include/configs/hymod.h > include/configs/LANTEC.h > include/configs/gth2.h > include/configs/dreamplug.h > include/configs/lacie_kw.h > include/configs/dockstar.h > include/configs/omap3_evm_common.h > include/configs/guruplug.h > include/configs/rd6281a.h > include/configs/ep8260.h > include/configs/sheevaplug.h Nevermind, I just checked all of these and they do indirectly set CONFIG_DOS_PARTITION where necessary. > Can I just modify those board configs to set CONFIG_DOS_PARTITION > and remove the duplicated FAT-specific partition-table probing > code or is there something else I should do? I will be submitting a patch shortly. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Cannot access a FAT filesystem in an El-Torito partition
The U-Boot FAT driver appears to manually check for the existence of an MS-DOS partition table, even when CONFIG_DOS_PARTITION is present and working. As a result, it is not possible to use the FAT driver on an ISO9660 El-Torito boot volume, because it does not have a DOS MBR and does not pass the magic number check. It looks like the code in the FAT driver to check DOS MBRs is just legacy code from before libpart existed, and I would like to remove it, except several board configs seem to set CONFIG_CMD_FAT but do not set CONFIG_DOS_PARTITION: include/configs/dbau1x00.h include/configs/mv88f6281gtw_ge.h include/configs/hymod.h include/configs/LANTEC.h include/configs/gth2.h include/configs/dreamplug.h include/configs/lacie_kw.h include/configs/dockstar.h include/configs/omap3_evm_common.h include/configs/guruplug.h include/configs/rd6281a.h include/configs/ep8260.h include/configs/sheevaplug.h Can I just modify those board configs to set CONFIG_DOS_PARTITION and remove the duplicated FAT-specific partition-table probing code or is there something else I should do? Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] net/eth: Don't issue warnings for offboard ethernet chips
On Dec 17, 2011, at 15:16, Wolfgang Denk wrote: > Dear Kyle Moffett, > In message <1324001821-15337-1-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> When using an offboard ethernet chip such as e1000, it is highly likely >> that the driver has already read a valid MAC address from the onboard >> EEPROM. In that case, U-Boot should not issue a warning about the >> absence of an "eth*addr" value in the environment. > > Yes, it should. The rule is that then environment settings always > have precedence, and if they are missing or contain different data > than other sources for this information, a waning shall be printed. That is a problem for devices which are typically add-in PCI cards. In that case U-Boot can't be expected to have knowledge of what the MAC addresses should be, and it should just use the ROM attached to the card instead. In our use case the e1000 chips are on a separate board attached via CompactPCI. U-Boot should not spontaneously start throwing errors just because the board was stuck into a different slot or replaced due to hardware failure. I was careful to maintain the behavior that U-Boot will issue a warning if there is no usable MAC address or if the environment has a MAC address that does not match the board. However, in the case that the board itself has a valid external MAC address and U-Boot does not even have an environment variable, it should not cause extra messages. Think about hot-pluggable USB net adapters where the detection order is nondeterministic. >> A properly configured HWW-1U-1A board is fixed from this output: >> >> Net: e1000: 00:50:93:81:ff:8a >> e1000: 00:50:93:81:ff:8b >> owt0, owt1, peer, e1000#0 >> Warning: failed to set MAC address >> , e1000#1 >> Warning: failed to set MAC address >> >> To this: >> >> Net: e1000: 00:50:93:81:ff:8a >> e1000: 00:50:93:81:ff:8b >> owt0, owt1, peer, e1000#0, e1000#1 > > This is also not correct. There should never be any printing of the > MAC addresses here. > > The messages should be: > > Net: owt0, owt1, peer, e1000#0, e1000#1 The "e1000" driver has always done that. I can submit a separate patch to fix that if you would like. >> Furthermore, the log messages should avoid screwing up the "Net:" output >> formatting provided by the calling code, EG: >> >> Net: eth0, eth1 [could not set MAC: 00:50:93:81:ff:8a], eth2 > > No. "could not set" is an error message, and deserves a separate > line. Ok, I will respin the patch so that errors show up like this: Net: eth0, eth1, ERROR: Could not set MAC address: 00:50:93:81:ff:8a eth2 Is that OK? Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Allow the "reset" command to be omitted with CONFIG_CMD_RESET
On Dec 16, 2011, at 14:30, Mike Frysinger wrote: > On Friday 16 December 2011 13:49:15 Moffett, Kyle D wrote: >> On Dec 16, 2011, at 00:05, Mike Frysinger wrote: >>> On Thursday 15 December 2011 22:32:41 Kyle Moffett wrote: >>>> This new #define is set in config_cmd_defaults.h (which is automatically >>>> included on every board by "mkconfig"), but this allows boards to elect >>>> to omit the "reset" command if necessary with "#undef CONFIG_CMD_RESET". >>> >>> would be a good time to split this into a dedicated common/cmd_reset.c, >>> we can also then fix cmd_boot.c to only build when CONFIG_CMD_GO is >>> enabled >> >> The only references to CONFIG_CMD_GO are in config_cmd_defaults.h, README, >> and common/cmd_boot.c, there is nothing that ever turns it off, so perhaps >> the config symbol should just be removed? > > i have some boards that turn it off Ok, I really don't see the point in moving these 5 lines to their own file, especially since they are in is already just 78 lines long: U_BOOT_CMD( reset, 1, 0,do_reset, "Perform RESET of the CPU", "" ); If you really think that's what I should do, though, I'll go ahead and make that change. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Allow the "reset" command to be omitted with CONFIG_CMD_RESET
On Dec 16, 2011, at 00:05, Mike Frysinger wrote: > On Thursday 15 December 2011 22:32:41 Kyle Moffett wrote: >> This new #define is set in config_cmd_defaults.h (which is automatically >> included on every board by "mkconfig"), but this allows boards to elect >> to omit the "reset" command if necessary with "#undef CONFIG_CMD_RESET". > > NAK: doesn't seem to address the feedback i posted last time ... Hmm, I thought I addressed these: >> --- a/include/config_cmd_defaults.h >> +++ b/include/config_cmd_defaults.h > > updating these files is not sufficient and will break boards. drop the hunks > to > these files and update include/config_defaults.h instead. The "mkconfig" program automatically includes both config_defaults.h and config_cmd_defaults.h, so this is sufficient to not break boards. This is exactly how CONFIG_CMD_GO works. Since it's always on, there's no need to add it to config_cmd_all.h either (which is why I removed that hunk). > would be a good time to split this into a dedicated common/cmd_reset.c, > we can also then fix cmd_boot.c to only build when CONFIG_CMD_GO is enabled The only references to CONFIG_CMD_GO are in config_cmd_defaults.h, README, and common/cmd_boot.c, there is nothing that ever turns it off, so perhaps the config symbol should just be removed? Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] tools/setlocalversion: Update from the Linux Kernel
On Dec 16, 2011, at 11:07, Mike Frysinger wrote: > On Thursday 15 December 2011 21:13:55 Kyle Moffett wrote: >> The version from the kernel is not directly usable as it has code for >> supporting CONFIG_LOCALVERSION from Kconfig, but the version that was >> imported is very similar to the one in Linux v3.2-rc4. > > NAK: this breaks localversion-* support Ah, I missed that somehow. I'd read through the U-Boot version and did not remember seeing that part so I assumed it wasn't necessary. > ignoring that, i'm not sure you really need to delete the CONFIG_xxx > handling. > under u-boot, they'd expand to like "" which would end at the same code. by > keeping as much code as possible in common, it should make future updates > easier. Ok, I will respin, retest, and resubmit as you suggest. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] powerpc: Minimal private libgcc to build on Debian
Ping? Is this patch acceptable for merging? Cheers, Kyle Moffett On Oct 18, 2011, at 17:16, Moffett, Kyle D wrote: > Standard Debian powerpc and powerpcspe systems only include hard-float > libgcc in their native compilers, which causes scary build warnings when > building U-Boot. > > While U-Boot does not use floating point at all, it is perfectly legal > for a hard-float libgcc to use features of the floating point unit to > implement 64-bit divides and shifts. Since U-Boot does not set up the > floating point unit or initialize the registers, that could cause > crashes or other misbehavior. > > In particular, on e500 systems with SPE extensions, a "hard-float" > libgcc might use the top half of the "extended" 64-bit registers for > various 64-bit shifts and other operations, which is probably unsafe. > > To avoid a dependency on "nof" (IE: soft-float) libgcc provided by the > system toolchain, U-Boot should be able to provide its own copies of the > necessary routines just like the Linux kernel does. In particular, this > resolves build failures on native Debian PowerPC systems which no longer > provide any soft-float libraries in the system toolchain. > > This patch provides an implementation of USE_PRIVATE_LIBGCC=yes from the > Linux Kernel that matches what the ARM architecture implements in > assembly files in arch/arm/lib/*.S > > Specifically, the routines are: _ashldi3(), _ashrdi3(), and _lshrdi3(). > They were borrowed from arch/powerpc/kernel/misc_32.S as of v2.6.38-rc5, > commit 85e2efbb1db9a18d218006706d6e4fbeb0216213, and are GPLv2+. > > The Makefile framework was copied from the U-Boot ARM port. > > Signed-off-by: Kyle Moffett > Cc: Wolfgang Denk > Cc: Kim Phillips > Cc: Andy Fleming > Cc: Kumar Gala > Cc: Stefan Roese > > --- > > I believe I have addressed all of the previous questions regarding > this patch with an updated commit message. Can this be merged? > > Cheers, > Kyle Moffett > > -- > Curious about my work on the Debian powerpcspe port? > I'm keeping a blog here: http://pureperl.blogspot.com/ > --- > arch/powerpc/lib/Makefile | 23 - > arch/powerpc/lib/_ashldi3.S | 45 + > arch/powerpc/lib/_ashrdi3.S | 47 +++ > arch/powerpc/lib/_lshrdi3.S | 45 + > 4 files changed, 159 insertions(+), 1 deletions(-) > create mode 100644 arch/powerpc/lib/_ashldi3.S > create mode 100644 arch/powerpc/lib/_ashrdi3.S > create mode 100644 arch/powerpc/lib/_lshrdi3.S > > diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile > index 724d8ee..cdd62a2 100644 > --- a/arch/powerpc/lib/Makefile > +++ b/arch/powerpc/lib/Makefile > @@ -23,6 +23,19 @@ > > include $(TOPDIR)/config.mk > > +## Build a couple of necessary functions into a private libgcc > +LIBGCC = $(obj)libgcc.o > +GLSOBJS += _ashldi3.o > +GLSOBJS += _ashrdi3.o > +GLSOBJS += _lshrdi3.o > +LGOBJS := $(addprefix $(obj),$(GLSOBJS)) \ > +$(addprefix $(obj),$(GLCOBJS)) > + > +## But only build it if the user asked for it > +ifdef USE_PRIVATE_LIBGCC > +TARGETS += $(LIBGCC) > +endif > + > LIB = $(obj)lib$(ARCH).o > > SOBJS-y += ppccache.o > @@ -53,9 +66,14 @@ endif > > COBJS += $(sort $(COBJS-y)) > > -SRCS := $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c) > +SRCS := $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \ > +$(SOBJS-y:.o=.S) $(COBJS-y:.o=.c) > OBJS := $(addprefix $(obj),$(SOBJS-y) $(COBJS-y)) > > +TARGETS += $(LIB) > + > +all: $(TARGETS) > + > $(LIB): $(obj).depend $(OBJS) > @if ! $(CROSS_COMPILE)readelf -S $(OBJS) | grep -q '\.fixup.*PROGBITS';\ > then \ > @@ -65,6 +83,9 @@ $(LIB): $(obj).depend $(OBJS) > fi; > $(call cmd_link_o_target, $(OBJS)) > > +$(LIBGCC): $(obj).depend $(LGOBJS) > + $(call cmd_link_o_target, $(LGOBJS)) > + > # > > # defines $(obj).depend target > diff --git a/arch/powerpc/lib/_ashldi3.S b/arch/powerpc/lib/_ashldi3.S > new file mode 100644 > index 000..e452f56 > --- /dev/null > +++ b/arch/powerpc/lib/_ashldi3.S > @@ -0,0 +1,45 @@ > +/* > + * This code was copied from arch/powerpc/kernel/misc_32.S in the Linux > + * kernel sources (commit 85e2efbb1db9a18d218006706d6e4fbeb0216213, also > + * known as 2.6.38-rc5). The source file copyrights are as follows: > + * > + * (C) Copyright 1995-1996 Gary Thomas (g...@linuxppc.org) > + * > + * Largely rewritten by Cort Dougan (c...@cs.nmt.edu) >
[U-Boot] Build breakage due to "Standalone Apps: Standalone apps should only need exports.h"
Hi, I was just rebasing and tweaking my board-support patch to send out again and I noticed that the latest U-Boot master branch does not build when the config option "CONFIG_CMD_SPI" is enabled: In file included from exports.c:41 [...]/include/_exports.h: In function 'jumptable_init': [...]/include/_exports.h:27: error: 'spi_init' undeclared (first use in this function) [...]/include/_exports.h:27: error: (Each undeclared identifier is reported only once [...]/include/_exports.h:27: error: for each function it appears in.) [...]/include/_exports.h:28: error: 'spi_setup_slave' undeclared (first use in this function) [...]/include/_exports.h:29: error: 'spi_free_slave' undeclared (first use in this function) [...]/include/_exports.h:30: error: 'spi_claim_bus' undeclared (first use in this function) [...]/include/_exports.h:31: error: 'spi_release_bus' undeclared (first use in this function) [...]/include/_exports.h:32: error: 'spi_xfer' undeclared (first use in this function) >From what I can tell, the only way to "fix" the exports.h header would be to copy-paste much of "include/spi.h" (including the structure and most of the constants) into "include/exports.h". Basically, if those SPI functions should be usable from outside code then the structure definition and the flags to be passed in all need to be available to the outside code as well. Locally I just reverted the patch, but I'm not sure what the desired long-term fix should be, since it seems silly to have to duplicate spi.h in exports.h Cheers, Kyle Moffett -- Kyle Moffett eXMeritus Software Integrated Intelligence The Boeing Company (703) 764-0925 (703) 812-1146 [FAX] kyle.d.moff...@boeing.com ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] E1000 build warnings
On Nov 04, 2011, at 12:47, Wolfgang Denk wrote: > you have been modifyong the E1000 driver lately so I hope you are in > the best position to help and fix a number of build warnings. > > For example when building for the MVBC_P board, I get this: > > e1000.c: In function 'e1000_read_mac_addr': > e1000.c:1149:2: warning: dereferencing type-punned pointer will break > strict-aliasing rules [-Wstrict-aliasing] > e1000.c:1149:2: warning: dereferencing type-punned pointer will break > strict-aliasing rules [-Wstrict-aliasing] Hmm, I would be inclined to just use "-fno-strict-aliasing" and "-Wno-strict-aliasing" here. The Linux kernel does the exact same thing because strict-aliasing tends to cause arbitrary breakage with things like memcpy() and network protocol buffers. Here's one relevant email thread: https://lkml.org/lkml/2003/2/25/270 > e1000.c: In function 'e1000_reset_hw': > e1000.c:1373:11: warning: variable 'icr' set but not used > [-Wunused-but-set-variable] In this case the "icr" variable should just be removed; the register read is just to clear some read-once bits. I'll submit a patch next week to fix this one if nobody else beats me to it. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] e1000: fix bugs from recent commits
On Oct 29, 2011, at 15:33, Wolfgang Denk wrote: > Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more > information" failed to initialize the checksum variable which should > result in random results. Fix that. > > Commit 2326a94d caused a ton of "unused variable 'x'" warnings. > Fix these. While we are at it, remove some bogus parens. > > Signed-off-by: Wolfgang Denk > Cc: Kyle Moffett Tested-by: Kyle Moffett Thanks for doing this; my apologies for missing these issues in the first place. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] e1000: fix bugs from recent commits
On Oct 28, 2011, at 01:49, Wolfgang Denk wrote:
> Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
> information" failed to initialize the checksum variable which should
> result in random results. Fix that.
> [I wonder if that code has _ever_ been tested!!]
>
> I wonder if you have ever actually build and run this code???
> With the "checksum" variable being random (due to not being
> initialized) you should have seen serious checksum problems.
> How did this escape your testing?
Yes, sorry, that is the correct fix.
I'm running my old GIT checkout unmodified on my hardware right now
with no problems; I must have just gotten lucky with the compiler's
register allocation, perhaps?
*pokes through disassembly*
Yeah, it looks like I just got lucky and that stack slot is always
overwritten with zero by an earlier function call.
> Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
> Fix these. While we are at it, remove some bogus parens.
>
> And all these build warnings - have you ever actully compiled that
> code? What's going on here??? - wd
>
> #define E1000_WRITE_FLUSH(a) \
> - do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
> + E1000_READ_REG(a, STATUS)
The correct E1000_WRITE_FLUSH macro should be:
#define E1000_WRITE_FLUSH(a) \
do { uint32_t x = E1000_READ_REG(a, STATUS); (void)x; } while(0)
It shouldn't return a value, it's just ensuring that writes are properly
posted to the PCI bus.
I booted them and ran some standard load-and-performance tests on our
boards. Unfortunately I was not paying enough attention to the build
log after splitting the e1000 patches to put the SPI code in a separate
file; that was when I removed the apparently-unnecessary "(void)x;".
Is there a good way to turn on "-Werror" for U-Boot builds by default?
I'll make sure to do that in the future to catch these problems first.
Ohh, actually, for some reason "-Wno-unused" was getting propagated in
to CFLAGS from my system environment variables, that's why I didn't see
the error originally.
My apologies for the bugs.
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Allow the "reset" command to be omitted with CONFIG_CMD_RESET
On Oct 20, 2011, at 15:53, Mike Frysinger wrote: > On Thursday 20 October 2011 15:05:50 Kyle Moffett wrote: >> --- a/common/cmd_boot.c >> +++ b/common/cmd_boot.c >> @@ -71,8 +71,10 @@ U_BOOT_CMD( >> >> #endif >> >> +#ifdef CONFIG_CMD_RESET >> U_BOOT_CMD( >> reset, 1, 0,do_reset, >> "Perform RESET of the CPU", >> "" >> ); >> +#endif > > would be a good time to split this into a dedicated common/cmd_reset.c Is it really worth a separate file for just 7 lines of code? The "do_reset" function itself is defined in the architecture-specific code that implements it (EG: arch/powerpc/cpu/mpc85xx/cpu.c), so there is nothing else generic about the command. >> --- a/include/config_cmd_all.h >> +++ b/include/config_cmd_all.h >> >> --- a/include/config_cmd_defaults.h >> +++ b/include/config_cmd_defaults.h > > updating these files is not sufficient and will break boards. drop the hunks > to > these files and update include/config_defaults.h instead. Ah, ok. The README docs weren't very clear about that. Fixed, thanks! Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] bootm: Use "panic()" in non-recoverable error conditions
On Oct 20, 2011, at 15:31, Wolfgang Denk wrote: > Dear Kyle Moffett, > In message <1319134031-28503-1-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> All of these errors are various kinds of fatal memory overwrite >> conditions and so should be handled by panic(). This fixes a bug in >> which the error message might not get all the way out to the serial >> console before the system reboots; panic() has a built-in delay after >> doing a printf() before calling do_reset(). >> >> This will result in a change in behavior for the 27 board configuration >> files which set CONFIG_PANIC_HANG (less than 5% of the total). They >> will now hang in those fatal error conditions instead of trying to >> reboot. >> >> Given that CONFIG_PANIC_HANG is intended to prevent the system from >> rebooting after it has encountered an unrecoverable error, this seems to >> be the desired behavior for those 27 board configurations. > > This is your interpretation, but the users and especially the > respective board maintainers may think different. We should at least > try and get feedback from them first. Well, to be fair, the README says this about the config option: CONFIG_PANIC_HANG Define this variable to stop the system in case of a fatal error, so that you have to reset it manually. This is probably NOT a good idea for an embedded system where you want the system to reboot automatically as fast as possible, but it may be useful during development since you can try to debug the conditions that lead to the situation. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] mpc85xx: Add a board-specific restart hook
On Oct 20, 2011, at 10:02, Wolfgang Denk wrote:
> Dear "Moffett, Kyle D",
> In message <8b4ac84d-1f22-4326-b75a-fb3cc39a5...@boeing.com> you wrote:
>>
>> Would you accept a patch which makes it possible for a board to not
>> implement a "reset" command at all?
>>
>> There are a few places in common/cmd_bootm.c which are converted to use
>> panic("...") instead of printf("...")+do_reset().
>
> This is not acceptable, as changes behaviour: panic() will halt the
> system, not reset it.
That is obviously wrong, as a 5 second glance at panic() in the file
lib/vsprintf.c would tell you. For 540 of the 567 board configs in
the include/configs/ directory, panic will directly call do_reset();
only 27 of the configs (less than 5%) set CONFIG_PANIC_HANG.
The only change with the patch for boards without CONFIG_PANIC_HANG is
that panic() has an extra udelay() to ensure that the serial console
messages go out before the reset.
For the boards that *do* set CONFIG_PANIC_HANG, none of the fatal
errors in common/cmd_bootm.c should cause U-Boot to reset, they are
all valid panic() conditions, such as GZIP overwrite errors and fatal
image format issues. In those cases this is also a bugfix.
Patch to follow shortly.
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] mpc85xx: Add a board-specific restart hook
On Oct 19, 2011, at 20:15, Mike Frysinger wrote: > On Wednesday 19 October 2011 18:52:10 Moffett, Kyle D wrote: >> So U-Boot MUST NOT reset without negotiating, even if the current CPU has >> crashed, as that will cause the other (possibly perfectly operational) CPU >> to also crash. > > so why can't you have your do_reset() board hook negotiate with the other CPU > to reset the system ? That's what I originally implemented. The problem is the negotiation can take an unbounded amount of time to execute, so if it's run from the command line then it needs to be interruptible (EG: with Ctrl-C), which means it needs to be able to return an error. I can see if I can come up with yet another possible alternative, but I'd like to try to get the less-controversial patches (especially the GPIO header and the e1000 ones) merged first. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] mpc85xx: Add a board-specific restart hook
On Oct 19, 2011, at 17:55, Mike Frysinger wrote:
> On Wednesday 19 October 2011 17:05:12 Moffett, Kyle D wrote:
>> On Oct 19, 2011, at 16:35, Wolfgang Denk wrote:
>>> Moffett, Kyle D wrote:
>>>> Since "reset" is basically just like any other U-Boot shell command,
>>>
>>> No, it ain't.
>>>
>>>> (except with some side-effects) it seems reasonable to allow a board
>>>> handler to return an error instead of resetting.
>>>
>>> No. Reset will never return. It is supposed to hard reset the board,
>>> which means crossing the point of no return.
>>
>> Ok.
>>
>> By that definition, my board cannot safely use U-Boot's "reset" command.
>>
>> Would you accept a patch which makes it possible for a board to not
>> implement a "reset" command at all?
>>
>> There are a few places in common/cmd_bootm.c which are converted to use
>> panic("...") instead of printf("...")+do_reset().
>
> i guess i'm dumb, but i don't understand why it can't be made to work on your
> board. but worse comes to worse, you could have your board reset call hang()
> or panic() ...
Each board has two separate processor units on it, each with their own memory
and devices. If either processor reboots on its own then both will hang and
be unusable.
So U-Boot MUST NOT reset without negotiating, even if the current CPU has
crashed, as that will cause the other (possibly perfectly operational) CPU to
also crash.
I suppose in theory I could add the __board_restart() hook and make it call
"panic()", but by removing do_reset() entirely for my board, I get helpful
link errors for tracking down places which should be calling panic() or
whatever instead.
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] mpc85xx: Add a board-specific restart hook
On Oct 19, 2011, at 16:35, Wolfgang Denk wrote:
> Dear "Moffett, Kyle D",
>
> In message <7c2673d7-cc5c-490c-9809-06c9a2071...@boeing.com> you wrote:
>>
>> Since "reset" is basically just like any other U-Boot shell command,
>
> No, it ain't.
>
>> (except with some side-effects) it seems reasonable to allow a board
>> handler to return an error instead of resetting.
>
> No. Reset will never return. It is supposed to hard reset the board,
> which means crossing the point of no return.
Ok.
By that definition, my board cannot safely use U-Boot's "reset" command.
Would you accept a patch which makes it possible for a board to not
implement a "reset" command at all?
There are a few places in common/cmd_bootm.c which are converted to use
panic("...") instead of printf("...")+do_reset().
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] mpc85xx: Add a board-specific restart hook
On Oct 18, 2011, at 23:20, Mike Frysinger wrote:
> On Tuesday 18 October 2011 19:41:23 Kyle Moffett wrote:
>> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> unsigned long val, msr;
>>
>> +/* Allow boards to override the reset */
>> +int err = __board_restart();
>> +if (err)
>> +return err;
>
> i thought we decided that do_reset() shouldn't return
> -mike
For our hardware we have to coordinate reset between both CPUs on the
same physical board, so a "reset" command may hang indefinitely waiting
for the other CPU (IE: If it refuses to shutdown in Linux or is running
U-Boot).
So for user convenience I need to be able to Ctrl-C the communication.
Since "reset" is basically just like any other U-Boot shell command,
(except with some side-effects) it seems reasonable to allow a board
handler to return an error instead of resetting.
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] mpc85xx: Add inline GPIO acessor functions
On Oct 19, 2011, at 09:51, Kumar Gala wrote: > On Oct 18, 2011, at 10:19 PM, Mike Frysinger wrote: >> On Tuesday 18 October 2011 19:41:22 Kyle Moffett wrote: >>> --- a/README >>> +++ b/README >>> >>> - 85xx CPU Options: >>> + CONFIG_MPC85XX_GENERIC_GPIO >>> + >>> + Provide a generic gpio_request() interface to the onboard >>> + processor GPIOs in the MPC85XX-series chips. >> >> do you really need a CONFIG knob for this ? defining this adds no overhead >> that i can see. > > I agree, wasn't paying attention that this is just in a .h, so lets remove > the CONFIG_ option bits (both in README and in .h) Well, I was concerned that this API could conflict with another provider of the generic GPIO interface (IE: An I2C GPIO chip or whatever). Looking further it appears that only 3-4 drivers implement the generic GPIO interface, and all of those are CPU-specific, so it should not be a problem in practice. The reason I originally made this configurable was because I did not want or need the generic GPIO interface; I needed an API that let me set a large group of GPIOs simultaneously. During an early review it was asked why I didn't use the standard interface; so I implemented the simple wrapper functions. I've removed the CONFIG_* option and will be resubmitting that patch shortly. >>> --- /dev/null >>> +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h >> >> missing #ifdef protection against multiple inclusion > > please fix. Really? Oops! I have no idea how I missed that. Fixed. Cheers, Kyle Moffett -- Curious about my work on the Debian powerpcspe port? I'm keeping a blog here: http://pureperl.blogspot.com/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divides on32-bit PowerPC
On Jul 29, 2011, at 17:46, York Sun wrote:
> On Fri, 2011-07-29 at 16:26 -0500, Moffett, Kyle D wrote:
>> On Jul 29, 2011, at 14:46, York Sun wrote:
>>> On Fri, 2011-07-29 at 11:35 -0700, York Sun wrote:
>>>> On Fri, 2011-07-29 at 12:20 -0500, Moffett, Kyle D wrote:
>>>>> On Jul 28, 2011, at 17:41, York Sun wrote:
>>>>>> I found a problem with the round up. Please try
>>>>>>
>>>>>> picos_to_mclk(mclk_to_picos(3))
>>>>>>
>>>>>> You will notice the result is round up. I am sure it is unexpected.
>>>>>
>>>>> Hmm... what does fsl_ddr_get_mem_data_rate() return on your system?
>>>>> I cannot reproduce this here with my memory freq of 800MHz.
>>>>>
>>>>> [...snip...]
>>>>>
>>>>> Obviously you could also get rid of the rounding alltogether, but I
>>>>> don't know why it was put in the code in the first place...
>>>>
>>>> I think the problem comes from the round up. But your calculation of
>>>> remainder is not right.
>>
>> Can you tell me what your value of get_ddr_freq() is? I may be able to
>> reproduce the issue here.
>
> 26400
Ok, so that's a 264MHz DDR frequency, right? 264 * 10^6?
If I plug that number in to the code I can reproduce your result:
picos_to_mclk(mclk_to_picos(3)) == 4
But that happens with both the new code *AND* the old code. I ran them
side-by-side in the same test-harness:
OLD:
get_memory_clk_period_ps(): 7580
picos_to_mclk(mclk_to_picos(3)): 4
--
NEW:
get_memory_clk_period_ps(): 7580
picos_to_mclk(mclk_to_picos(3)): 4
So there's a bug, but it's always been there...
Just to show the floating-point math really quick:
2 * (10^12 picosec/sec) / (264 * 10^6 cyc/sec)
== 7575.757575. picoseconds
Since the code rounds to the nearest 10ps, we get 7580ps. Then 3 mclk is
22740ps, but it should be 22727.272727.
The picos_to_mclk() function always rounds up, because we must avoid
undercutting the SPD timing margin. If your SPD specifies a minimum of a
7590ps timing window, you MUST use a 2 mclk delay (even though it's
painfully close to 1 mclk), because otherwise you will get RAM errors.
Therefore mclk_to_picos() and get_memory_clk_period_ps() should always
round down, even though they currently round to the closest 10ps.
Continuing the example: If we later we do picos_to_mclk(22740):
22740ps * (264 * 10^5 cyc/sec) * 0.5 / (10^12 picosec/sec)
== 3.00168
When we do the integer math with remainder, we get:
3 rem 336000
In both cases, we correctly automatically round up to 4.
I believe I have a fix for the problem; here's my fixed test case log:
get_memory_clk_period_ps(): 7575
picos_to_mclk(mclk_to_picos(3)): 3
And here is the new code (the picos_to_mclk() function is unchanged):
unsigned int mclk_to_picos(unsigned int mclk)
{
unsigned int data_rate = get_ddr_freq(0);
/* Be careful to avoid a 64-bit full-divide (result fits in 32) */
unsigned long long mclk_ps = ULL_2E12 * mclk;
do_div(mclk_ps, data_rate);
return mclk_ps;
}
unsigned int get_memory_clk_period_ps(void)
{
return mclk_to_picos(1);
}
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divides on32-bit PowerPC
On Jul 29, 2011, at 14:46, York Sun wrote:
> On Fri, 2011-07-29 at 11:35 -0700, York Sun wrote:
>> On Fri, 2011-07-29 at 12:20 -0500, Moffett, Kyle D wrote:
>>> On Jul 28, 2011, at 17:41, York Sun wrote:
>>>> I found a problem with the round up. Please try
>>>>
>>>> picos_to_mclk(mclk_to_picos(3))
>>>>
>>>> You will notice the result is round up. I am sure it is unexpected.
>>>
>>> Hmm... what does fsl_ddr_get_mem_data_rate() return on your system?
>>> I cannot reproduce this here with my memory freq of 800MHz.
>>>
>>> The function has obviously always done rounding, even before I made
>>> any changes. Have you retested what the math does with the patch
>>> reverted to see if it makes any difference?
>>>
>>> Hmm, looking at it, the obvious problem case would be mclk_to_picos()
>>> using a rounded result in a subsequent multiply. Can you retest by
>>> replacing mclk_to_picos() with the following code; this should keep
>>> the error down by doing the rounding-to-10ps *after* the multiply.
>>>
>>> unsigned int mclk_to_picos(unsigned int mclk)
>>> {
>>>unsigned int data_rate = get_ddr_freq(0);
>>>unsigned int result;
>>>
>>>/* Round to nearest 10ps, being careful about 64-bit multiply/divide
>>> */
>>>unsigned long long mclk_ps = ULL_2e12 * mclk;
>>>
>>>/* Add 5*data_rate, for rounding */
>>>mclk_ps += 5*(unsigned long long)data_rate;
>>>
>>>/* Now perform the big divide, the result fits in 32-bits */
>>>do_div(mclk_ps, data_rate);
>>>result = mclk_ps;
>>>
>>>/* We still need to round to 10ps */
>>>return 10 * (result/10);
>>> }
>>>
>>> Obviously you could also get rid of the rounding alltogether, but I
>>> don't know why it was put in the code in the first place...
>>
>> I think the problem comes from the round up. But your calculation of
>> remainder is not right. You explained to me with example of 2*10^^12-1.
>> It seems to be right until I tried another number 264*10^^7. I think the
>> following calculation of remainder is
>>
>> clks_rem = do_div(clks, UL_5POW12);
>> clks >>= 13;
>> clks_rem |= (clks & (UL_2POW13-1)) * UL_5POW12;
That code you pasted is definitely not what is in the tree.
The in-tree code is:
57 /* First multiply the time by the data rate (32x32 => 64) */
58 clks = picos * (unsigned long long)get_ddr_freq(0);
59
60 /*
61 * Now divide by 5^12 and track the 32-bit remainder, then divide
62 * by 2*(2^12) using shifts (and updating the remainder).
63 */
64 clks_rem = do_div(clks, UL_5pow12);
65 clks_rem <<= 13;
66 clks_rem |= clks & (UL_2pow13-1);
67 clks >>= 13;
68
69 /* If we had a remainder, then round up */
70 if (clks_rem)
71 clks++;
Can you tell me what your value of get_ddr_freq() is? I may be able to
reproduce the issue here.
Also, can you try your test case with the replacement for mclk_to_picos()
that I pasted above and let me know what happens?
I think the *real* error is that get_memory_clk_period_ps() (which I did
not change) rounds up or down to a multiple of 10ps, and then the *result*
of get_memory_clk_period_ps() is multiplied by the number of clocks. The
effect is that the 5ps of rounding error that can occur is multiplied by
the number of clocks, and may result in a much-too-large result. When you
convert that number of picoseconds *back* to memory clocks you get a
result that is too big, but that's because it was too many picoseconds!
Where did you get the number 264*10^7? That seems too big to be a number
of picoseconds (that's 2ms!), but way too small to be the product of
get_ddr_freq() and some number of picoseconds.
EG: For my system here with a DDR frequency of 800MHz, each clock is 2500ps,
so 3 clocks times 2500ps times 800MHz is 6*10^12.
In fact if you give "picos_to_mclk()" any number of picoseconds larger than
at least 1 clock, the initial value of the "clks" variable is guaranteed to
be at least 2*10^12, right?
Using my number, let's run through the math:
So if you start with picos = 7499 (to put rounding to the test) and
get_ddr_freq() is 800MHz:
clks = 7499 * 800*1000*1000;
Now we have "clks" = 59992, and we divide by 5**12:
clks_rem = do_div(clks, UL_5pow12);
Since 59992/(5**12) is 24572.7232, we get:
clks = 24572
clks_rem = 176562500 (which is equal to 5^12 * 0.7232)
Now left-shift clks_rem by 13:
clks_rem = 14464
Th
Re: [U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divides on32-bit PowerPC
On Jul 28, 2011, at 17:41, York Sun wrote:
> On Mon, 2011-03-14 at 16:35 -0500, Moffett, Kyle D wrote:
>> On Mar 14, 2011, at 16:22, York Sun wrote:
>>> On Wed, 2011-02-23 at 11:35 -0500, Kyle Moffett wrote:
>>>> + * Now divide by 5^12 and track the 32-bit remainder, then divide
>>>> + * by 2*(2^12) using shifts (and updating the remainder).
>>>> + */
>>>> + clks_rem = do_div(clks, UL_5pow12);
>>>> + clks_rem <<= 13;
>>>
>>> Shouldn't this be clks_rem >>= 13 ?
>>>>
>>>> + clks_rem |= clks & (UL_2pow13-1);
>>>> + clks >>= 13;
>>>> +
>>>> + /* If we had a remainder, then round up */
>>>> + if (clks_rem)
>>>>clks++;
>>>> - }
>
> I found a problem with the round up. Please try
>
> picos_to_mclk(mclk_to_picos(3))
>
> You will notice the result is round up. I am sure it is unexpected.
Hmm... what does fsl_ddr_get_mem_data_rate() return on your system?
I cannot reproduce this here with my memory freq of 800MHz.
The function has obviously always done rounding, even before I made
any changes. Have you retested what the math does with the patch
reverted to see if it makes any difference?
Hmm, looking at it, the obvious problem case would be mclk_to_picos()
using a rounded result in a subsequent multiply. Can you retest by
replacing mclk_to_picos() with the following code; this should keep
the error down by doing the rounding-to-10ps *after* the multiply.
unsigned int mclk_to_picos(unsigned int mclk)
{
unsigned int data_rate = get_ddr_freq(0);
unsigned int result;
/* Round to nearest 10ps, being careful about 64-bit multiply/divide */
unsigned long long mclk_ps = ULL_2e12 * mclk;
/* Add 5*data_rate, for rounding */
mclk_ps += 5*(unsigned long long)data_rate;
/* Now perform the big divide, the result fits in 32-bits */
do_div(mclk_ps, data_rate);
result = mclk_ps;
/* We still need to round to 10ps */
return 10 * (result/10);
}
Obviously you could also get rid of the rounding alltogether, but I
don't know why it was put in the code in the first place...
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv2] powerpc: Minimal private libgcc to build on Debian
On Apr 13, 2011, at 16:57, Wolfgang Denk wrote: > In message <1298479238-22114-1-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> Standard Debian powerpc and powerpcspe systems only include hard-float >> libgcc in their native compilers, which causes scary build warnings when >> building U-Boot. >> >> Debian and other PowerPC-supporting distributions used to provide libgcc >> and other libraries in a "nof" (soft-float) form in the "multilib" >> packages. As they were completely unused by the distribution and >> therefore tended to be very buggy it was decided to save some time on >> the part of the maintainers and build-servers by removing them. >> >> Admittedly, right now the linker warnings do not indicate any problems, >> as the included routines do not use any floating point at all. >> >> The concern is that if floating-point code were ever added it might >> cause hard-float code to be unexpectedly included in U-Boot without >> generating a hard error. This would cause unexplained crashes or >> indeterminate results at runtime. >> >> The easiest way to resolve this is to borrow the routines that U-Boot >> needs from the Linux kernel, which has the same issue. >> >> Specifically, the routines are: _ashldi3(), _ashrdi3(), and _lshrdi3(). > > Sorry, but I cannot follow your logic. > > First, we do not use floating point in U-Boot. We don't. Period. > [The only exception being well-designed and hand-crafted assemby code > where it is unavoidable - for example in the POST code to test the > FPU, or for certain atomic 64 bit stores]. > > So FP support should never be a reason for such a change. The concern is not with floating point being used in U-Boot, but with the internal implementation of those libgcc functions. Specifically, I could very well imagine that functions such as the full 64-bit divide (_udivdi3) might internally be optimized by using specific floating point operations on PowerPC. On PowerPC with SPE extensions, the native libgcc might use the full 64-bit extensions to the regular integer registers to perform such operations. > What confuses me completely is why you then add some shift functions, > which are completely unrelated to FP operations. Since that CPU state is not necessarily set up in U-Boot, we should not use functions from a libgcc which is allowed to use those ops. Since a native PowerPC U-Boot built on a PowerPC Debian system does not have a soft-float libgcc to build against, this patch provides a minimal "libgcc" with the few libgcc functions that U-Boot seems to need (the various 64-bit shifts). This is similar to the minimal "libgcc" provided by the ARM arch from arch/arm/lib/_ash[lr]di3.S and arch/arm/lib/_divsi3.S, and is almost identical to what the Linux kernel uses on PowerPC. This is only enabled if USE_PRIVATE_LIBGCC=1 (the same config variable as the ARM private libgcc) to avoid breaking other ports. I will update the patch notes. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/5] e1000: New "e1000" commands for SPI EEPROM management
On Apr 13, 2011, at 01:23, Wolfgang Denk wrote: > In message <0ef7e520-ff4a-435f-af2a-0d47c0951...@boeing.com> you wrote: >> In particular, those other eeprom drivers simply have a single hardcoded >> I/O base address that they assume is properly mapped, IE: >>> struct eth_device dev; >>> dev.iobase = CONFIG_SMC911X_BASE; >> >> That won't really work with the way the existing E1000 driver is set >> up; it expects to run with full PCI device access using the standard >> U-Boot PCI calls. It seems to get very confused if you poke at the >> hardware behind its back. > > I'm not really sure how this is related to the EEPROM access part of > the code. This is functionally separate from the network driver, > isn't it? > > And I don't really see where the EEPROM part needs to be PCI aware. No, the EEPROM driver relies upon the network driver having initialized the hardware correctly (including mapping PCI BARs, etc). The SPI bus to the EEPROM is also shared with the network hardware and firmware, and requires the use of other shared arbitration register bits. Given that the E1000 is always a PCI device, I can't see why you would ever build an E1000 EEPROM programmer which was *not* PCI aware. I honestly have no clue what IO address the E1000 BARs actually get mapped at, nor do I really care; that is (and should always be) entirely abstracted away by the U-Boot PCI layer. >> Finally, from an actual operational standpoint, this EEPROM driver is >> designed to allow the user to program up to 64kB of information to the >> E1000 EEPROM, including manageability firmware and other stuff; speed >> and copy-to/from-memory operations are very important. > > These should be important to all kinds of drivers. I agree, but for the other ethernet eeprom drivers I don't think it matters. From what I understand the existing programmers support at most 256-byte EEPROMs (because that is what the hardware supports). The E1000 is unique among the ethernet eeprom drivers in U-Boot because of the size of the chip. >> The other EEPROM "apps" only really support changing individual bytes >> one-at-a-time and reprogramming the MAC address, which is insufficient >> for initial hardware load. > > I wonder if we could / should unify all this code. It looks like most of the programmers right now use SPI, although from what I remember of the Linux code some of the E1000 chipsets use a different register interface to program other kinds of Flash memory (I think the embedded E1000 in the ICH/PCH chipsets do this). The only real obstacle right now is that there can only be one SPI host driver in U-Boot at compile-time. To fix this we would need to create a "struct spi_host" abstraction with function pointers to allow multiple different hosts to be registered as bus-0, bus-1, etc. Then the "EEPROM" commands could just talk to the appropriate device on each SPI bus. >> If you agree then I will modify the patch to move the new code into a >> separate e1000_eeprom.c file which is conditionally compiled, but >> still linked into the main U-Boot image. > > That's OK with me. [As long as it remains optional.] Ok, I'll make the necessary changes and resubmit. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/5] e1000: New "e1000" commands for SPI EEPROM management
On Apr 12, 2011, at 16:24, Wolfgang Denk wrote: > In message <1297467482-14864-5-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> For our new board ports, we are programming the EEPROMs attached to our >> Intel 82571EB controllers from software (using U-Boot and Linux). >> >> This code provides a helpful set of "e1000" subcommands for performing >> EEPROM manipulation on e1000 devices, including displaying a hex-dump, >> copying to and from main memory, and verifying/updating of the software >> checksum. >> >> The following commands work for programming the EEPROM from USB: >> usb start >> fatload usb 0 $loadaddr 82571EB_No_Mgmt_Discrete-LOM.bin >> e1000 0 eeprom program $loadaddr 0 1024 >> e1000 0 eeprom checksum update >> >> Please keep in mind that the Intel-provided .eep files are organized as >> 16-bit words. When converting them to binary form for programming you >> must byteswap each 16-bit word so that it is in little-endian form. >> >> This means that when reading and writing words to the SPI EEPROM, the >> bit ordering for each word looks like this on the wire: > > I don't like the idea of having this in the driver code itself. It is > a separate function, which should be implemented in a separate source > file. > > Eventually this should not even be linked with U-Boot, but kept > separate as lodable module, like eepro100_eeprom.c, smc911x_eeprom.c > and smc9_eeprom.c. Hmm, there seem to be some fairly significant functionality differences between this e1000 driver and those other EEPROM drivers. In particular, those other eeprom drivers simply have a single hardcoded I/O base address that they assume is properly mapped, IE: > struct eth_device dev; > dev.iobase = CONFIG_SMC911X_BASE; That won't really work with the way the existing E1000 driver is set up; it expects to run with full PCI device access using the standard U-Boot PCI calls. It seems to get very confused if you poke at the hardware behind its back. Also, I can't see any way to allow CONFIG_CMD_SPI to link against an external "app" (the functionality provided by patch 5/5). Finally, from an actual operational standpoint, this EEPROM driver is designed to allow the user to program up to 64kB of information to the E1000 EEPROM, including manageability firmware and other stuff; speed and copy-to/from-memory operations are very important. The other EEPROM "apps" only really support changing individual bytes one-at-a-time and reprogramming the MAC address, which is insufficient for initial hardware load. If you agree then I will modify the patch to move the new code into a separate e1000_eeprom.c file which is conditionally compiled, but still linked into the main U-Boot image. Thanks for your comments! Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/5] e1000: Restructure and streamline PCI device probing
On Apr 12, 2011, at 16:17, Wolfgang Denk wrote:
> In message <1297467482-14864-3-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> By allocating the e1000 device structures much earlier, we can easily
>> generate better error messages and siginficantly clean things up.
>>
>> The only user-visable change (aside from reworded error messages) is
>> that a detected e1000 device which fails to initialize due to software
>> or hardware error will still be allocated a device number.
>>
>> As one example, consider a system with 2 e1000 PCI devices where the
>> first controller has a corrupted EEPROM. Using the old code the
>> second controller would be "e1000#0", while with this change it would be
>> "e1000#1".
>>
>> This change should hopefully make such EEPROM errors much more
>> straightforward to handle correctly in boot scripts and the like.
>>
>> It is also necessary for a followup patch which allows SPI programming
>> of an e1000 controller's EEPROM even if the checksum is invalid.
>
> This patch has a number of overlong lines. Please globally fix the
> line length.
The only lines longer than 80 characters in this patch are these 4 when
indented by 3 levels:
> hw->hw_addr = pci_map_bar(devno, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
> printf("%s: ERROR: Can't enable I/O memory\n", nic->name);
> printf("%s: ERROR: Can't enable bus-mastering\n", nic->name);
> printf("%s: ERROR: EEPROM checksum is bad!\n", nic->name);
According to Documentation/CodingStyle:
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are placed
> substantially to the right. The same applies to function headers with a long
> argument list. Long strings are as well broken into shorter strings. The
> only exception to this is where exceeding 80 columns significantly increases
> readability and does not hide information.
Wrapping any of those lines will in fact make them much harder to read and
dissimilar to the other nearly identical printf() calls in that piece of
code; I strongly disagree that it is necessary.
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v6 4/4] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices
On Mar 21, 2011, at 18:24, Wolfgang Denk wrote: > In message you wrote: >> >> I apparently cannot rely on the U-Boot *CODE* to understand what the >> U-Boot *CODING* style is. > > You don't have to rely on the code. It's clearly documented. > > The README says: > > Coding Standards: > - > > All contributions to U-Boot should conform to the Linux kernel > coding style; see the file "Documentation/CodingStyle" and the script > "scripts/Lindent" in your Linux kernel source directory... > > http://www.denx.de/wiki/U-Boot/CodingStyle says the same. So here it is claimed that the U-Boot coding style is the same as the Linux Kernel coding style. But previously you said: WARNING: line over 80 characters #463: FILE: board/exmeritus/hww1u1a/hww1u1a.c:136: +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> >>> This one is only a warning, and it's much more readable to have 1 >>> 84-character line than split it across 2 different lines. Even still, >>> this warning is only issued if you pass "--subjective" to checkpatch, which >>> is documented to "enable more subjective tests". > > In U-Boot, it is considered to be an ERROR. So which is it? If U-Boot uses the official Linux Kernel CodingStyle, then a few >80-char lines are OK if it increases readability, for example by not having to wrap a simple U_BOOT_CMD function declaration that goes just a whole 4 characters over the limit. If U-Boot does NOT use the Linux Kernel CodingStyle and wants to refuse all patches with over 80 characters then you should copy checkpatch.pl and the CodingStyle document and change that wording from "strongly preferred" to "hard requirement", and change the "WARNING" to "ERROR". > And the referred document says: > Chapter 2: Breaking long lines and strings > > Coding style is all about readability and maintainability using > commonly > available tools. > > The limit on the length of lines is 80 columns and this is a strongly > preferred limit. > > Statements longer than 80 columns will be broken into sensible chunks. > > Now what exactly is unclear here? You removed the very next paragraph in the Linux CodingStyle file, which contains: > The only exception to this is where exceeding 80 columns significantly > increases > readability and does not hide information. Furthermore, Linus Torvalds himself said in an email: >> We fixed that to allow checkpatch to skip those warnings, but the fact is, >> the fundamnetal problem has always been the "80 character" part. >> >> I don't think any kernel developers use a vt100 any more. And even if they >> do, I bet they curse the "24 lines" more than they curse the occasional >> 80+ character lines. >> >> I'd be ok with changing the warning to 132 characters, which is another >> perfectly fine historical limit. Or we can split the difference, and say >> "ok, 106 characters is too much". I don't care. But 80 characters is >> causing too many idiotic changes. >> >> There are way worse problems in many patches than long lines. Too complex >> expressions. Too deep indentation. Pure crap code. People seem to get way >> too hung up on ".. but at least it passes checkpatch". The line you were complaining about is a static U_BOOT_CMD function declaration for goodness sakes! It's about as common as dirt in the U-Boot code and the only reason it doesn't fit on one line anymore is because I made the name of the function *NAME* about 8 characters longer in this version of the patch than it was in a previous patch. It's not any more complex than it was before, nor is it any harder to read. I'm tired and fed up with this whole mess. If you think it's likely to be accepted I'll go ahead and submit one more final respin tomorrow, assuming I feel up to it. Otherwise, I wish you the best of luck with U-Boot. Good night. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v6 4/4] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices
On Mar 21, 2011, at 17:34, Wolfgang Denk wrote: > In message you wrote: >> >> Just looking at the last ~200 commits (actually 187, because it ignores >> merges): >> >> $ git format-patch -o recent-patches -200 origin/master >> $ ./checkpatch.pl --no-tree --strict recent-patches/* >checkpatch.log 2>&1 >> $ grep 'over 80 char' checkpatch.log | wc -l >> 130 >> >> That's 130 lines in the last 200 patches which are over 80 characters?!?! >> How are those patches any different from mine? > > The difference is: They were not detected. > > Patches welcome. If those were patches from two years ago and your style policies had significantly changed since then I would understand. But those are patches from *LAST MONTH* which you were perfectly happy to merge from dozens of different developers, but had *I* submitted identical patches they would have been rejected without even a second glance. That is inconsistent at best, and in my humble opinion downright rude. >> Look, I'm really trying to comply with U-Boot coding standards, but I'm >> really of pissed off about the inconsistent requirements you are applying >> to my patches versus a lot of other things that YOU ARE MERGING on a >> regular basis. > > The requirements are NOT inconsistent. It's just that nobody is > perfect, and nobody ever claimed that we manage to get 100% of review > coverage. I give up. I apparently cannot rely on the U-Boot *CODE* to understand what the U-Boot *CODING* style is. The time investment to get reasonable board support merged into U-Boot is proving to be *greater* than the time investment to just maintain our board ports out of tree. If anyone would like to use our code as a reference or try to get it merged themselves, I will continue to maintain our GPLed out-of-tree patchset here: http://opensource.exmeritus.com/git/ But otherwise I see no valid reason I should waste any more of my time submitting patches which get torn apart out of hand over issues which are completely ignored for patches which come in from other maintainers. In the future we will have to weigh other boot-loader alternatives due to the unfriendly attitude here. Good luck. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v6 4/4] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices
On Mar 21, 2011, at 16:30, Wolfgang Denk wrote:
> In message <5b9d9c87-c278-4af3-b20c-26ecff6c0...@boeing.com> you wrote:
>>
>>> WARNING: line over 80 characters
>>> #463: FILE: board/exmeritus/hww1u1a/hww1u1a.c:136:
>>> +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char *
>>> const argv[])
>>
>> This one is only a warning, and it's much more readable to have 1 84-charac
>
> In U-Boot, it is considered to be an ERROR.
Just looking at the last ~200 commits (actually 187, because it ignores merges):
$ git format-patch -o recent-patches -200 origin/master
$ ./checkpatch.pl --no-tree --strict recent-patches/* >checkpatch.log 2>&1
$ grep 'over 80 char' checkpatch.log | wc -l
130
That's 130 lines in the last 200 patches which are over 80 characters?!?!
How are those patches any different from mine?
$ grep '^ERROR:' checkpatch.log | wc -l
113
And that's 113 HARD ERRORS from checkpatch!?!?!
Of those, 32 are "Missing Signed-off-by: line(s)", 20 are "macros with
complex values should be enclosed in parenthesis", 19 are inconsistent
or missing whitespace issues, 4 are "(foo*) instead of "(foo *)", 3 are
invalid UTF-8 errors, 4 are "return is not a function" errors, etc, etc.
Look, I'm really trying to comply with U-Boot coding standards, but I'm really
of pissed off about the inconsistent requirements you are applying to my
patches versus a lot of other things that YOU ARE MERGING on a regular basis.
So why are you picking on my board-specific code so hard here? This is
extremely frustrating for me and a strong deterrent against us *ever*
contributing to U-Boot again in the future.
>> ter line than split it across 2 different lines. Even still, this
>> warning is only issued if you pass "--subjective" to checkpatch, which
>> is documented to "enable more subjective tests".
>
> No, I get this warning without such flags. The command I run is just
> "checkpatch.pl --no-tree"
What version of checkpatch are you running? I copied version 0.31 out of
my latest Linux kernel tree, which identical to the latest version from
here:
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/
If U-Boot policy is to run checkpatch then you'd better either specify a
particular version and command-line options or be willing to accept the
default output of the latest version.
> BTW - could you please restrict your line length to some 70 characters
> or so? Thanks.
Sorry about that, sending email through an Exchange server is no fun :-(.
Hopefully I've got it fixed.
+U_BOOT_CMD(
+ hww1u1a_test_cpu_a, 1, 0, do_hww1u1a_test_cpu_a,
+ "Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
+ /* */" && \n"
+ "hww1u1a_test_cpu_a || \n"
>>> =20
>>> What is this empty comment needed for?
>>
>> Just a mental placeholder for the fact that the U_BOOT_CMD macro inserts
>> the name of the command in that spot. Will remove.
>
> We don't provide usage examples in the help text. This should be
> fixed in the first place.
This *IS* the help text, and not a sample usage. This is visually and
effectively no different from this text in common/cmd_mp.c:
U_BOOT_CMD(
cpu, CONFIG_SYS_MAXARGS, 1, cpu_cmd,
"Multiprocessor CPU boot manipulation and release",
" reset - Reset cpu \n"
"cpu status- Status of cpu \n"
"cpu disable - Disable cpu \n"
"cpu release [args] - Release cpu at with
[args]"
#ifdef CPU_ARCH_HELP
"\n"
CPU_ARCH_HELP
#endif
);
+ /* Turn on the "HRESET_REQ" pin (hard-reset request) */
+ printf("\nRESET: Hardware reset triggered, waiting...\n");
+ out_be32(&gur->rstcr, 0x2);
+ while (1)
+ udelay(1);
+ }
>>> Should that not be an infinite wait here?
>>
>> At this point if the board does not reset due to hardware failure it's
>> better off hanging than silently falling through.
>
> Why don't you simply call "hang()" then?
Didn't know it existed at the time the code was written. Will fix.
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v6 4/4] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices
Wolfgang,
Thanks for your detailed reviews!
Once I get these last few style issues resolved, what more do I need to do to
get this merged? I don't really want to spam the list with more nearly
identical copies of these patches unless I'm sure that all the necessary review
items have been taken care of.
On Mar 15, 2011, at 15:36, Wolfgang Denk wrote:
> In message <1300208664-18339-5-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> The eXMeritus HWW-1U-1A unit is a DO-160-certified 13lb 1U chassis
>> with 3 independent TEMPEST zones. Two independent P2020 computers may
>> be found inside each zone. Complete hardware support is included.
>
> Please run checkpatch on your submissions!
I did run it on this patch, although I forgot to run it on the resurrected
reset patch (which is also now fixed). It's a common gripe on the LKML that
the tool is overzealous about certain warnings in cases where the "fix" makes
the code less readable.
Specifically:
> ...
>> +/* Ok, now go ahead and program all of those in one go */
>> +mpc85xx_gpio_set( gpio_high|gpio_low|gpio_in,
>> +gpio_high|gpio_low,
>> +gpio_high);
>
> ERROR: space prohibited after that open parenthesis '('
> #427: FILE: board/exmeritus/hww1u1a/hww1u1a.c:100:
> + mpc85xx_gpio_set( gpio_high|gpio_low|gpio_in,
I could "fix" this code to read:
mpc85xx_gpio_set(gpio_high|gpio_low|gpio_in,
gpio_high|gpio_low, gpio_high);
And it would be much harder to visually compare the three bitmask arguments
against each other.
>> +/*
>> + * If things have been taken out of reset early (for example, by one
>> + * of the BDI3000 debuggers), then we need to put them back in reset
>> + * and delay a while before we continue.
>> + */
>> +#define GPIO_RESETS (GPIO_DIMM_RESET|GPIO_USB_RESET|GPIO_GETH0_RESET)
>> +if (mpc85xx_gpio_get(GPIO_RESETS)) {
>
> Please don;t add #defines right in the middle of the code.
Fixed, thanks!
>> +/*
>> + * This little shell function just returns whether or not it's CPU A.
>> + * It can be used to select the right device-tree when booting, etc.
>> + */
>> +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char *
>> const argv[])
>
> WARNING: line over 80 characters
> #463: FILE: board/exmeritus/hww1u1a/hww1u1a.c:136:
> +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
This one is only a warning, and it's much more readable to have 1 84-character
line than split it across 2 different lines. Even still, this warning is only
issued if you pass "--subjective" to checkpatch, which is documented to "enable
more subjective tests". This thread discusses it further:
http://lkml.org/lkml/2009/12/15/490
>> +U_BOOT_CMD(
>> +hww1u1a_test_cpu_a, 1, 0, do_hww1u1a_test_cpu_a,
>> +"Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
>> +/* */" && \n"
>> +"hww1u1a_test_cpu_a || \n"
>
> What is this empty comment needed for?
Just a mental placeholder for the fact that the U_BOOT_CMD macro inserts the
name of the command in that spot. Will remove.
>> +/* Now the serial# part of the hostname */
>> +for (j = 0; serialnr[j]; j++)
>> +if (isalnum(serialnr[j]))
>> +hww1u1a_prompt[i++] = tolower(serialnr[j]);
>
> Braces needed for multiline statements.
Fixed, thanks!
>> +/* Turn on the "HRESET_REQ" pin (hard-reset request) */
>> +printf("\nRESET: Hardware reset triggered, waiting...\n");
>> +out_be32(&gur->rstcr, 0x2);
>> +while (1)
>> +udelay(1);
>> +}
>
> Should that not be an infinite wait here?
At this point if the board does not reset due to hardware failure it's better
off hanging than silently falling through.
>> +/* Enable the U-Boot "memory test" */
>> +#define CONFIG_SYS_MEMTEST_START 0x
>> +#define CONFIG_SYS_MEMTEST_END 0x7fff
>
> I think this has not been tested, right?
I'm pretty sure it's been tested, but not very recently; the memory on all the
shipped units is ECC anyways.
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divides on32-bit PowerPC
On Mar 14, 2011, at 16:22, York Sun wrote: > On Wed, 2011-02-23 at 11:35 -0500, Kyle Moffett wrote: >> + * Now divide by 5^12 and track the 32-bit remainder, then divide >> + * by 2*(2^12) using shifts (and updating the remainder). >> + */ >> +clks_rem = do_div(clks, UL_5pow12); >> +clks_rem <<= 13; > > Shouldn't this be clks_rem >>= 13 ? >> >> +clks_rem |= clks & (UL_2pow13-1); >> +clks >>= 13; >> + >> +/* If we had a remainder, then round up */ >> +if (clks_rem) >> clks++; >> -} Since I'm dividing a second time, the old remainder value represents the high bits of the new remainder value and therefore needs to be left-shifted, then the now-empty low bits of the remainder are taken from the bits of "clks" which get shifted away. Example: Say I want to divide 1 (IE: 2*10^^12 - 1) by 2 (2*10^^12). Obviously the dividend is less than the divisor (by 1), so the result should be 0 and the remainder should be equal to the dividend. So my initial value is: clks = 1; Now I divide by 5^^12: clks_rem = do_div(clks, 244140625); /* This number is 5pow12 */ The results are: clks_rem == 244140624 clks == 8191 Now I shift left: clks_rem <<= 13; And get: clks_rem == 11808 Finally, I copy the low bits of clks to clks_rem and shift them out of clks: clks_rem |= clks & (UL_2pow13-1); clks >>= 13 The result is as expected: clks_rem == 1; clks == 0; Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/21] Define new system_restart() and emergency_restart()
On Mar 14, 2011, at 16:38, Wolfgang Denk wrote: > In message <613c8f89-3ce5-4c28-a48e-d5c3e8143...@boeing.com> you wrote: >> >> If just *one* of the 2 CPUs triggers the reset then only *some* of >> the attached hardware will be properly reset due to a hardware >> errata, and as a result the board will sometimes hang or corrupt DMA >> transfers to the SSDs shortly after reset. > ... >> Yes, it's a royal pain, but we're stuck with this hardware for the >> time being, and if the board can't communicate then it might as well >> hang() anyways. > > Do you agree that this is a highly board-specific problem (I would > call it a hardware bug, but I don't insist you agree on that term), > and while there is a the need for you to work around such behaviour > there is little or no reason to do this, or anything like that, in > common code ? Oh, absolutely. I do think there still needs to be a separation between a "normal user-initiated restart" and an "panic-time emergency restart" though, see further on in this email. The comment about Ctrl-C was simply because our board restart can be aborted by someone at the console because it may take a while for the other CPU to cleanly shut down and acknowledge. Our __board_restart() watches for Ctrl-C to handle this nicely, and then returns an error, which makes do_reset() return to the user or script with an error. Obviously in the middle of a panic() there is nothing useful to return to, so the code keeps polling regardless. Such decisions on what is and is not "acceptable" to run on a panic() are better left to the individual boards and architectures. Specifically, the separate board and arch hooks for regular and "emergency" restarts that I included in the patch: __arch_restart() __board_restart() __arch_emergency_restart() __board_emergency_restart() >>> And if there are more things that could be done to provide a "better" >>> reset, then why should we not always do these? >> >> If the board is in a panic() state it may well have still-running DMA >> transfers (such as USB URBs), or be in the middle of writing to >> FLASH. > > The same (at least having USB or other drivers still being enabled, > and USB writing it's SOF counters to RAM) can happen for any call to > the reset() function. I see no reason for assuming there would be > better or worse conditions to perform a reset. I would argue that is a bug to be fixed. Regardless of how various boards and architectures implement "reset", U-Boot should provide generic functionality to drivers and library code to allow them to indicate what they want: (1) A safe normal operational restart, with all hardware shut down (as much as is considered necessary on the platform). Depending on the platform this may fail or take some time. (2) A critical error restart, where system state may be undefined and the calling code does not expect the function to ever return. Linux has *both* of those cases in the kernel: sys_reboot() and emergency_restart(). >> If this board starts receiving packets and then panic()s, it will >> disable address translation and immediately re-relocate U-Boot into >> RAM, then zero the BSS. If the network card tries to receive a packet >> after BSS is zeroed, it will read a packet buffer address of >> (probably) 0x0 from the RX ring and promptly overwrite part of >> U-Boot's memory at that address. > > Agreed. So this should be fixed. One clean way to fix it would be to > help improving the driver model for U-Boot (read: create one) and > making sure drivers get deinitialized in such a case. This another excellent reason to have separate system_restart() and emergency_restart(). The "system_restart()" function would hook into the driver model and perform device shutdown (just like Linux), while the emergency_restart() function (and therefore panic()) would not. The "jump to _start" case is very similar to Linux kexec(). There are two specific use-cases: (1) Safe reliable run-time handoff from one kernel to another (2) Emergency panic() call into another kernel to record the error and reboot safely In the former case, the kernel runs all of its normal shutdown handlers and quiesces all DMA before passing control to the new kernel. Under U-Boot this is analogous to the "system_restart()" function I added. The "system_restart()" function would be the ideal central place to hook driver-model device shut-down. In the latter case (similar to "emergency_restart()"), the new kernel runs in a sub-section of memory *completely-preallocated* at boot time, and explicitly ignores all other memory because there might be ongoing DMA transactions. The "emergency kexec" case intentionally avoids running device shutdown handlers because the system state is not well defined. Under U-Boot there is no way to run completely from a preallocated chunk of memory, which means that a panic()-time jump to "_start" is not safe. Cheers, Kyle Moffett ___
Re: [U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divides on32-bit PowerPC
On Mar 14, 2011, at 14:19, York Sun wrote: > On Wed, 2011-02-23 at 11:35 -0500, Kyle Moffett wrote: >> The current FreeScale MPC-8xxx DDR SPD interpreter is using full 64-bit >> integer divide operations to convert between nanoseconds and DDR clock >> cycles given arbitrary DDR clock frequencies. >> >> Since all of the inputs to this are 32-bit (nanoseconds, clock cycles, >> and DDR frequencies), we can easily restructure the computation to use >> the "do_div()" function to perform 64-bit/32-bit divide operations. >> >> This decreases compute time rather significantly for each conversion and >> avoids bringing in a very complicated function from libgcc. >> >> It should be noted that nothing else in U-Boot or the Linux kernel seems >> to require a full 64-bit divide on any 32-bit PowerPC. >> >> Build-and-boot-tested on the HWW-1U-1A board using DDR2 SPD detection. >> >> Signed-off-by: Kyle Moffett >> Cc: Andy Fleming >> Cc: Kumar Gala >> Cc: Wolfgang Denk >> Cc: Kim Phillips >> --- >> >> Ok, so this patch touches a rather sensitive part of the fsl_ddr logic. >> >> I spent a fair amount of time trying to verify that the resulting math is >> exactly the same as it was before, but the consequences of a mistake are >> insideous timing problems. Additional in-depth review would be much >> appreciated. >> > > What's the gain of the changes? A smaller footprint? Going forward from > 32- to 64-bit, are we going to change it back? On 64-bit this change is basically a no-op, because do_div() is implemented as a literal 64-bit divide operation and the instruction scheduling works out almost the same. On 32-bit PowerPC a fully accurate 64/64 divide (__udivdi3 in libgcc) is 1.1kb of code and hundreds or thousands of dependent cycles to compute, all of which is linked in from libgcc. Another 1.2kb of code comes in for __umoddi3. The original reason I wrote this patch is that the native "libgcc" on my boards is hard-float and therefore generates warnings when linking to it from soft-float code. The toolchain is the native Debian powerpc (or powerpcspe), and most other native PowerPC distributions are also hard-float-only. When I combine this patch with the other patch I posted to create a minimal internal libgcc with a few 64-bit shift functions from the linux kernel, I can successfully build U-Boot on those native PowerPC systems. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divideson32-bit PowerPC
On Mar 14, 2011, at 15:41, York Sun wrote: > Kyle, > > On Mon, 2011-03-14 at 14:04 -0500, Moffett, Kyle D wrote: >> On 64-bit this change is basically a no-op, because do_div() is implemented >> as a literal 64-bit divide operation and the instruction scheduling works >> out almost the same. >> >> On 32-bit PowerPC a fully accurate 64/64 divide (__udivdi3 in libgcc) is >> 1.1kb of code and hundreds or thousands of dependent cycles to compute, all >> of which is linked in from libgcc. Another 1.2kb of code comes in for >> __umoddi3. >> >> The original reason I wrote this patch is that the native "libgcc" on my >> boards is hard-float and therefore generates warnings when linking to it >> from soft-float code. The toolchain is the native Debian powerpc (or >> powerpcspe), and most other native PowerPC distributions are also >> hard-float-only. >> >> When I combine this patch with the other patch I posted to create a minimal >> internal libgcc with a few 64-bit shift functions from the linux kernel, I >> can successfully build U-Boot on those native PowerPC systems. >> > > Points taken. Can you explain your algorithm? I see you want to do > clks/5^12/2^13, but I don't see when the clks is divided by 5^12. Did I > miss some lines? The "do_div()" macro is a bit confusing that way, it simultaneously performs an in-place division of the first argument and returns the remainder. Unfortunately that's a clumsy interface that's been around in the Linux kernel for ages. So this line here: clks_rem = do_div(clks, UL_5pow12); It divides clks by UL_5pow12 and saves the remainder as clks_rem. Then the 2^^13 divide and remainder is perform with shifts and masks. Actually, it occurs to me that the one comment isn't quite right, the remainder is a 64-bit value (not a 32-bit one). I feel relatively confident that this is the right technical direction to go, because almost all of the Linux kernel timekeeping and scheduling code uses 64-bit values and careful application of do_div() or shift+mask for efficiency. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/21] Define new system_restart() and emergency_restart()
On Mar 14, 2011, at 14:59, Wolfgang Denk wrote: > In message you wrote: >> My own board needs both processor modules to synchronize resets to allow >> them to come back up at all, which means that a "reset" may block for an >> arbitrary amount of time waiting for the other module to cleanly shut down >> and restart (or waiting for somebody to type "reset" on the other U-Boot). >> If someone just types "reset" on the console, I want to allow them to hit >> Ctrl-C to interrupt the process. > > This is not what the "reset" command is supposed to do. The reset > command is supposed to be the software equivalent of someone pressing > the reset button on your board - to the extend possible to be > implemented in software. On our boards, when the "reset" button is pressed in hardware, both processor modules on the board and all the attached hardware reset at the same time. If just *one* of the 2 CPUs triggers the reset then only *some* of the attached hardware will be properly reset due to a hardware errata, and as a result the board will sometimes hang or corrupt DMA transfers to the SSDs shortly after reset. The only way to reset either chip safely is by resetting both at the same time, which requires them to communicate before the reset and wait (possibly a long time) for the other board to agree to reset. Yes, it's a royal pain, but we're stuck with this hardware for the time being, and if the board can't communicate then it might as well hang() anyways. This same logic is also implemented in my Linux board-support code, so when one CPU requests a reset the other treats it as a Ctrl-Alt-Del. >>> What is the difference between these two - and why do we need >>> different functions at all? >>> >>> A reset is a reset is a reset, isn't it? >> >> That might be true *IF* all boards could actually perform a real hardware >> reset. >> >> Some can't, and instead they just jump to their reset vector (Nios-II) or to >> flash (some ppc 74xx/7xx systems). > > So this is the "reset" on these boards, then. > >> If the board just panic()ed or got an unhandled trap or exception, then you >> don't want to do a soft-reset that assumes everything is OK. A startup in >> a bad environment like that could corrupt FLASH or worse. Right now there >> is no way to tell the difference, but the lower-level arch-specific code >> really should care. > > I don't understand your chain of arguments. > > If there really is no better way to implement the reset on such > boards, then what else can we do? > > And if there are more things that could be done to provide a "better" > reset, then why should we not always do these? If the board is in a panic() state it may well have still-running DMA transfers (such as USB URBs), or be in the middle of writing to FLASH. Performing a jump to early-boot code which is only ever tested when everything is OK and devices are properly initialized is a great way to cause data corruption. I know for a fact that our boards would rather hang forever than try to reset without cooperation from the other CPU. >>> My initial feeling is a plain NAK, for this and the rest of the patch >>> series. Why would we want all this? >> >> While I was going through the hooks I noticed that several of them were >> explicitly NOT safe if the board was in the middle of a panic() for whatever > > Can you please peovide some specific examples? I don't understand what > you are talking about. Ok, using the ppmc7xx board as an example: /* Disable and invalidate cache */ icache_disable(); dcache_disable(); /* Jump to cold reset point (in RAM) */ _start(); /* Should never get here */ while(1) ; This board uses the EEPRO100 driver, which appears to set up statically allocated TX and RX rings which the device performs DMA to/from. If this board starts receiving packets and then panic()s, it will disable address translation and immediately re-relocate U-Boot into RAM, then zero the BSS. If the network card tries to receive a packet after BSS is zeroed, it will read a packet buffer address of (probably) 0x0 from the RX ring and promptly overwrite part of U-Boot's memory at that address. Depending on the initialization process and memory layout, other similar boards could start writing garbage values to FLASH control registers and potentially corrupt data. Since the panic() path is so infrequently used and tested, it's better to be safe and hang() on the boards which do not have a reliable hardware-level reset than it is to cause undefined behavior or potentially corrupt data. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/21] Define new system_restart() and emergency_restart()
Hi!
On Mar 13, 2011, at 15:24, Wolfgang Denk wrote:
> In message <1299519462-25320-2-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> In preparation for making system restart use a generic set of hooks for
>> boards and architectures, we define some wrappers and weak stubs.
>>
>> The new wrapper functions are:
>> system_restart() - Normal system reboot (IE: user request)
>> emergency_restart() - Critical error response (IE: panic(), etc)
>
> What is the difference between these two - and why do we need
> different functions at all?
>
> A reset is a reset is a reset, isn't it?
That might be true *IF* all boards could actually perform a real hardware reset.
Some can't, and instead they just jump to their reset vector (Nios-II) or to
flash (some ppc 74xx/7xx systems).
If the board just panic()ed or got an unhandled trap or exception, then you
don't want to do a soft-reset that assumes everything is OK. A startup in a
bad environment like that could corrupt FLASH or worse. Right now there is no
way to tell the difference, but the lower-level arch-specific code really
should care.
So system_restart() is what you use when the system is in a good normal
operating condition. The emergency_restart() is what gets called from panic()
or in other places where a crash has happened.
> ...
>> +/*
>> + * This MUST be called from normal interrupts-on context. If it requires
>> + * extended interaction with external hardware it SHOULD react to Ctrl-C.
>
> ??? Why such complicated restrictions for a simple command that is
> just supposed to reset the board?
This is just documenting what the underlying hardware-specific code is
guaranteed. On some hardware a "system_restart()" may fail and return -1, on
others the board needs to cleanly respond to interrupts while polling external
hardware, etc. This is analogous to the normal "sys_reboot()" code under
Linux, which requires interrupts-on, etc.
This is to contrast with the emergency_restart() function which has no such API
constraints. It can be called from any context whatsoever and will do its best
to either perform a full hardware reset or hang while trying.
>> + * If this function fails to guarantee a clean reboot or receives a Ctrl-C
>> + * keystroke it SHOULD return with an error (-1).
>
> A "reset" is supposed to take place immediately, and unconditionally.
> If you need delays and ^C handling and other bells and whistles,
> please add these to your own code, but not here.
There's no Ctrl-C handling anywhere in this code, it will all be in my own
__board_restart() hook. As above, this documentation is just describing the
guarantees provided to underlying __board_restart() and __arch_restart() hooks;
if they check for Ctrl-C while polling external hardware and return an error
then that's fine.
>> +int system_restart(void)
>> +{
>> +int err;
>> +
>> +/*
>> + * Print a nice message and wait a bit to make sure it goes out the
>> + * console properly.
>> + */
>> +printf ("Restarting...\n");
>> +udelay(5);
>> +
>> +/* First let the board code try to reboot */
>> +err = __board_restart();
>> +if (err)
>> +goto failed;
>> +
>> +/* Now call into the architecture-specific code */
>> +err = __arch_restart();
>> +if (err)
>> +goto failed;
>> +
>> +/* Fallback to the old do_reset() until everything is converted. */
>> +err = do_reset(NULL, 0, 0, NULL);
>> +
>> +failed:
>> +printf("*** SYSTEM RESTART FAILED ***\n");
>> +return err;
>> +}
>
> You are making a simple thing pretty much complicated. This adds lots
> of code and I cannot see any benefits.
No, it's actually a net removal of code, the diffstat is:
90 files changed, 341 insertions(+), 374 deletions(-)
The basic hook structure from system_restart() already existed individually in
6 different architectures (incl. PowerPC, Blackfin, ARM, MIPS...), each of
which had its own "board_reset()" or "_machine_restart()" or similar. This
code makes it entirely generic, so as a new board implementor you can simply
define a "__board_restart()" function to override (or enhance) the normal
architecture-specific code.
> My initial feeling is a plain NAK, for this and the rest of the patch
> series. Why would we want all this?
While I was going through the hooks I noticed that several of them were
explicitly NOT safe if the board was in the middle of a panic() for whatever
reason, so I split off the __*_emergency_restart() hooks separately to allow
architectures to handle them cleanly.
My own board needs both processor modules to synchronize resets to allow them
to come back up at all, which means that a "reset" may block for an arbitrary
amount of time waiting for the other module to cleanly shut down and restart
(or waiting for somebody to type "reset" on the other U-Boot). If someone just
types "reset" on the console, I want to allow them to hit Ctrl-C to inte
Re: [U-Boot] [PATCH 15/21] nios2: Generic system restart support
Hi! On Mar 08, 2011, at 19:13, Scott McNutt wrote: > Hi Kyle, > > Kyle Moffett wrote: >> The Nios-II port appears to use no generic hardware capability for >> performing a CPU reset. Since all of the supported boards use the exact >> same code to perform a jump-to-flash it goes into __arch_restart(). >> >> This means that Nios-II has a no-op __arch_emergency_restart() function. >> If the CPU is in an invalid state then jump-to-FLASH probably won't >> work. > > If the CPU state is stable enough to call __arch_emergency_restart(), > a jump to the reset address should be fine. In many implementations > the reset code is in on-chip ROM. If things get screwed up enough > to affect the code in on-chip ROM, it probably won't matter what > gets called. ;-) I'm not at all familiar with the Nios-II hardware platform. Is the on-chip ROM really safe to be called in arbitrary system states? Using FLASH memory as an example, consider a FLASH driver in the middle of a programming cycle when an unexpected exception occurs: * FLASH programming in process * CPU takes an unexpected trap * CPU calls exception vector (possibly with interrupts disabled or enabled) * emergency_restart() * __arch_emergency_restart() * Boot ROM is called Until the FLASH memory is reset and put back into a defined state it will be unusable, and if your boot process depends on the FLASH then serious problems will result. In that case it would be better not to just hang than try to restart. Basically the emergency_restart() code should handle being called even in the following scenarios: * Unexpected CPU exception or interrupt occurs * Interrupts on or off, or *from* an interrupt or trap handler. * FLASH or other peripherals in an undefined state If that's the case for your onboard ROM then I will certainly remove the no-op emergency restart hook from this patch. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 21/21] Remove legacy do_reset() function
Hi! On Mar 07, 2011, at 16:55, Graeme Russ wrote: > On Tue, Mar 8, 2011 at 4:37 AM, Kyle Moffett > wrote: >> All of the users of the legacy do_reset() function have been converted >> to __arch_restart() or __board_restart() as appropriate, so the >> compatibility calls to do_reset() may be removed. >> >> In addition the do_generic_reset() function is renamed to the now-unused >> name do_reset(). > > If it is unused, why not remove it completely? The "reset" command used to invoke "do_reset()". Partway through this patch series I changed it to invoke a new "do_generic_reset()" function to preserve bisection. The calltrace looked like this: => do_generic_reset() => system_restart() => __board_restart() => __arch_restart() => do_reset() Now that nothing talks about "do_reset()" anymore, I delete that old function and rename "do_generic_reset()" over top of it: => do_reset() [Same as old do_generic_reset()] => system_restart() => __board_restart() => __arch_restart() Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 11/21] i386: Generic system restart support
On Mar 07, 2011, at 17:26, Graeme Russ wrote:
> On Tue, Mar 8, 2011 at 9:06 AM, Moffett, Kyle D
> wrote:
>> On Mar 07, 2011, at 16:54, Graeme Russ wrote:
>>> This part does not make much sense - If the CPU is in 'a bad state' then
>>> it will probably be lights out anyway. As I understand it, an emergency
>>> restart is a restart not initiated by the user (divide by zero, unhandled
>>> exception etc), in which case i386 will make use of it
>>
>> I was considering unhandled exceptions, etc. to be "a bad state" :-D.
>>
>> Maybe I didn't explain it well enough in the patch summary, but basically
>> the default for "__arch_emergency_restart()" is to just call
>> "__arch_restart()". Since the i386 "__arch_restart()" function should work
>> fine even when U-Boot is in trouble, that architecture does not need to
>> override the default.
>>
>> Hopefully I am making sense now? Should I reword it from "when the CPU is
>> in a bad state" to "when U-Boot is in trouble", or is there something else
>> that would be easier to understand?
>
> I understand what you are doing now, thanks.
>
> I think you can scrap this part of the description and I will have i386
> start using __arch_emergency_restart() for 'Internal U-Boot Errors' such
> as divide by zero, unhandled exception, general protection faults etc
>
> I don't particularly like the 'emergency' naming - It's like if we don't
> do it things will blow up :) I think 'automatic' might be a closer term
The name "emergency_restart()" was borrowed from the Linux kernel; the kernel
shuts down disks and network interfaces prior to a regular restart but not
during an emergency restart. The best analogy is to the "EMERG" log level
(KERN_EMERG inside the Linux kernel).
Furthermore, you should *not* directly call __arch_emergency_restart(), that is
simply the architecture hook you provide to the generic code. Your exception
handlers should call "emergency_restart()" to allow board-specific code to
override the reboot, for example by triggering an onboard watchdog to reset
other hardware.
EG:
=> some_i386_trap_handler()
=> emergency_restart()
=> __board_emergency_restart()
=> __arch_emergency_restart()
=> do_reset() [The new, generic version]
=> system_restart()
=> __board_restart()
=> __arch_restart()
The __{board,arch}_restart(), etc are just predefined weak functions that can
be overridden by the implementation.
For example, my HWW-1U-1A board file effectively does this:
int __board_restart(void)
{
while (poll_external_software()) {
if (ctrlc())
return -1;
}
return 0;
}
void __board_emergency_restart(void)
{
while (poll_external_software_uninterruptible())
;
}
During a normal restart, I allow the polling to be interrupted with Ctrl-C, but
during an emergency restart I don't. In both cases, my function just *returns*
to allow the default MPC85xx code to actually perform the hardware-level
restart (by poking at the "reset request" bit in a CPU register).
> Is there anywhere yet where the code paths for the emergency and non
> emergency variants differ in the way they end up resetting the board?
There are several U-Boot board ports whose do_reset() functions (now called
"__board_restart()") just perform an unconditional jump to "_start" or a FLASH
"soft-reset" address. If the system has experienced an unexpected exception or
other problem then that is not safe, and it would be better to just hang().
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 11/21] i386: Generic system restart support
Hi! On Mar 07, 2011, at 16:54, Graeme Russ wrote: > On Tue, Mar 8, 2011 at 4:37 AM, Kyle Moffett > wrote: >> The i386 port has its own reset_cpu() dispatch for its various supported >> CPU families, so the existing do_reset() function is simply altered to >> use the new prototype for __arch_restart(). >> >> In addition, the debug message and delay are duplicated from the generic >> code, so they are removed. >> >> This reset code will probably work even when the CPU is in a bad state, >> so no separate __arch_emergency_restart() function is required. > > This part does not make much sense - If the CPU is in 'a bad state' then > it will probably be lights out anyway. As I understand it, an emergency > restart is a restart not initiated by the user (divide by zero, unhandled > exception etc), in which case i386 will make use of it I was considering unhandled exceptions, etc. to be "a bad state" :-D. Maybe I didn't explain it well enough in the patch summary, but basically the default for "__arch_emergency_restart()" is to just call "__arch_restart()". Since the i386 "__arch_restart()" function should work fine even when U-Boot is in trouble, that architecture does not need to override the default. Hopefully I am making sense now? Should I reword it from "when the CPU is in a bad state" to "when U-Boot is in trouble", or is there something else that would be easier to understand? Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/21] Define new system_restart() and emergency_restart()
On Mar 07, 2011, at 16:40, Mike Frysinger wrote:
> On Monday, March 07, 2011 12:37:22 Kyle Moffett wrote:
>> +__attribute__((__noreturn__))
>> +void emergency_restart(void)
>> +{
>> +__board_emergency_restart();
>> +__arch_emergency_restart();
>> +
>> +/* Fallback to the old do_reset() until everything is converted. */
>> +do_reset(NULL, 0, 0, NULL);
>> +
>> +printf("EMERGENCY RESTART: All attempts to reboot failed!");
>
> missing a trailing newline, and i'd just call puts() to bypass useless format
> string parsing
Ok, I'll go check for this in all the patches, thanks.
>> +udelay(5);
>
> this doesnt sit well with me. i dont see why this matters ... we dont have
> any delays today, and i dont think we should introduce any.
Actually, several architectures had a printf()+udelay() already. (i386 and
ARM, IIRC). Since I was moving the printf() to common code I figured I'd
better move the udelay() as well.
I believe they had comments to the effect of "This udelay() prevents garbage on
the serial console while rebooting", I would guess because it gives the FIFO
time to finish flushing.
I can move it back to the individual architectures if you'd like.
>> +__attribute__((__weak__))
>> +int __board_restart(void)
>> +{
>> +/* Fallthrough to architecture-specific code */
>> +return 0;
>> +}
>> +
>> +__attribute__((__weak__))
>> +int __arch_restart(void)
>> +{
>> +/* Fallthrough to legacy do_reset() code */
>> +return 0;
>> +}
>
> what use is there in having a return value ? the do_reset() funcs today dont
> return, which means they have no meaningful reset value.
Well, actually, a surprising number of them *do* return (at least on some board
ports).
In particular I'm doing this for a custom board of ours (HWW-1U-1A) which
*must* synchronize with Linux firmware running on another physical processor
prior to asserting the reset line, otherwise it probably won't boot up
correctly. Since the synchronization may take an arbitrary amount of time (IE:
Waiting for the other Linux to finish initializing), my board's
__board_restart() function checks for Ctrl-C on the serial console. If I
actually *do* get a Ctrl-C I need to prevent the __arch_restart() hook from
being run, by returning an error code.
The "emergency_restart()" best-effort reboot is obviously a special case, as
the system is already wedged and there's nowhere useful to return *to*.
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 1/4] Refactor do_reset() into board-specific and CPU-specific portions
On Feb 24, 2011, at 13:41, Wolfgang Denk wrote:
> In message you wrote:
>>
>> Perhaps the default should instead be something like this?
>>
>> __attribute__((__weak__)) int arch_reset(void)
>> {
>> while(1);
>> }
>
> No. Please don;t implement something that does not do what it is
> supposed to do. Rather make the problem obvious.
>
>> Having a default might make it easier to do the initial standup of a new CPU
>> type (less code to write initially), but you wouldn't get any obvious errors
>> about the fact that you are missing functionality.
>
> That's a fundamentally broken approach. You know how people work:
> they start, they have something running without too blatant error
> messages, they leave it as is and go on to the next task.
>
> I rather see a clear unresolved external reference error instead of a
> bad defualt that doesn't work.
Ok, I'm working on an updated patch.
There are a few CPU architectures like the 74xx_7xx which have worrying
comments like these:
/* no generic way to do board reset. simply call soft_reset. */
It basically just disables caching and performs a jump to the CPU reset vector.
Several boards already have overrides of various sorts to use board-specific
reset vectors; from looking at commit logs and configuration for the boards
based on the 74xx and 7xx I'm not convinced that reset function has ever been
tested on some of them.
I would almost be willing to argue that if that kind of "soft reset" is a
desirable feature then U-Boot should support it separately via a
"kexec()"-style interface.
Anyways, I should be sending out a new patch here shortly.
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 1/4] Refactor do_reset() into board-specific and CPU-specific portions
On Feb 23, 2011, at 14:35, Mike Frysinger wrote:
> On Wednesday, February 23, 2011 14:28:44 Kyle Moffett wrote:
>> +__attribute__((__weak__)) int arch_reset(void)
>> +{
>> +return 0;
>> +}
>
> is there any cpu which wouldnt provide arch_reset() ? i dont think it was
> possible in the past to do this, so i dont see any value in supporting this.
Hmm, good point.
Although, I think there are a few CPUs which fundamentally cannot reset on
their own (they rely on external board-specific hardware triggered by GPIOs).
Perhaps the default should instead be something like this?
__attribute__((__weak__)) int arch_reset(void)
{
while(1);
}
Having a default might make it easier to do the initial standup of a new CPU
type (less code to write initially), but you wouldn't get any obvious errors
about the fact that you are missing functionality.
I can't really form enough of an opinion to care either way, so if you have any
strong preferences please let me know.
Thanks for the review!
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/7] mpc85xx: Add inline GPIO acessor functions
On Feb 21, 2011, at 16:14, Wolfgang Denk wrote:
> In message <1298311199-18775-4-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> To ease the implementation of other MPC85xx board ports, several common
>> GPIO helpers are added to .
>
> In which way is this specific to 85xx? Why not make available on a
> wider base?
This particular set of registers is a particular part of the MPC85xx-series SOC.
The other boards based on mpc85xx seem to just do stuff like this:
volatile ccsr_gpio_t *pgpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR);
[...]
in_be32(&pgpio->gpdat);
[...]
out_be32(&pgpio->gpdat, newdat);
[...]
I thought that was kind of ugly and abstracted it out into static inline
functions in my board-specific header. Last time this was posted for review it
was pointed out that they are generally applicable to all MPC85xx chips ans so
I moved them to a more-generic header for other MPC85xx boards to use.
>> To assist other board ports, a small set of wrappers are used which
>> provides a standard gpio_request() interface around the MPC85xx-specific
>> functions. This can be enabled with CONFIG_MPC85XX_GENERIC_GPIO
>
> How similar is this interface with other "generic" GPIO interfaces we
> have here in U-Boot (and in Linux) ?
The interface emulates the "generic" GPIO API in Linux because Peter Tyser
indicated he would use that generic interface for his other MPC85xx board ports
if it was available. I don't actually use that particular API though; it isn't
really suitable to my board-support code and I'd rather delete it than try to
extend it further.
>> +static inline void mpc85xx_gpio_set(unsigned int mask,
>> +unsigned int dir, unsigned int val)
>> +{
>> +volatile ccsr_gpio_t *gpio;
>> +
>> +/* First mask off the unwanted parts of "dir" and "val" */
>> +dir &= mask;
>> +val &= mask;
>> +
>> +/* Now read in the values we're supposed to preserve */
>> +gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00);
>> +dir |= (in_be32(&gpio->gpdir) & ~mask);
>> +val |= (in_be32(&gpio->gpdat) & ~mask);
>> +
>> +/* Now write out the new values, writing the direction first */
>> +out_be32(&gpio->gpdir, dir);
>
> This way work, but quite often is wrong. If you set the direction
> first for an output, you start driving the old value, before you set
> the correct one. This results in a short pulse that may cause
> negative side effects.
>
> Don't!
Whoops, I did get that completely backwards! I was very carefully trying to
ensure a clean transition and managed to swap it :-(.
Will fix, thanks for noticing!
>> +asm("sync; isync":::"memory");
>
> Why would that be needed? out_be32() is supposed to provide sufficient
> memory barriers.
Again, I was just cribbing from some of the other board ports. If in_be32()
and out_be32() are defined to provide enough barriers then I'll remove it.
>> +static inline unsigned int mpc85xx_gpio_get(unsigned int mask)
>> +{
>> +volatile ccsr_gpio_t *gpio;
>
> Why would this volatile be needed here? [Please fix globally.]
As above this was copied from other MPC85xx boards, will fix.
>> +#ifdef CONFIG_MPC85XX_GENERIC_GPIO
>> +static inline int gpio_request(unsigned gpio, const char *label)
>> +{
>> +/* Do nothing */
>> +(void)gpio;
>> +(void)label;
>
> NAK. Please don't do that. Fix globally.
>
>> +static inline int gpio_direction_input(unsigned gpio)
>> +{
>> +mpc85xx_gpio_set_in(1U << gpio);
>> +return 0;
>> +}
>
> Why is this function not void when it cannot return any usefult return
> code anyway?
>
>> +static inline int gpio_direction_output(unsigned gpio, int value)
>> +{
>> +mpc85xx_gpio_set_low(1U << gpio);
>> +return 0;
>> +}
>
> Ditto.
This is the "Linux-compatible" gpio layer that Peter Tyser was asking for. I
sort-of-copied most of these functions from arch/nios2/include/asm/gpio.h,
which just does the "return 0;" in several functions.
Those 2 later functions are expected to be able to return an error (for I2C
chips and such). As I said above, if these wrappers are unacceptable then I
will just delete them; the only thing I use are the mpc85xx_gpio_*() functions.
Thanks for the review!
Cheers,
Kyle Moffett
smime.p7s
Description: S/MIME cryptographic signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/7] mpc8xxx: DDR2/3: Use human-readable SPD DIMM-type constants
On Feb 21, 2011, at 16:03, Wolfgang Denk wrote: > In message <1298311199-18775-3-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> Use #define constants to enhance readability of DDR2/3 SPD parsing code. >> Also add the DDR2 type for an SO-RDIMM module to the switch statement. >> >> Signed-off-by: Kyle Moffett >> --- >> arch/powerpc/cpu/mpc8xxx/ddr/ddr2_dimm_params.c | 34 +++-- >> arch/powerpc/cpu/mpc8xxx/ddr/ddr3_dimm_params.c | 37 >> ++- >> common/ddr_spd.c|2 +- >> include/ddr_spd.h | 28 - >> 4 files changed, 60 insertions(+), 41 deletions(-) > ... >> +case DDR3_SPD_MODULETYPE_UDIMM: >> +case DDR3_SPD_MODULETYPE_SO_DIMM: >> +case DDR3_SPD_MODULETYPE_MICRO_DIMM: >> +case DDR3_SPD_MODULETYPE_MINI_UDIMM: >> +/* Unbuffered DIMMs */ >> +pdimm->registered_dimm = 0; >> +pdimm->mirrored_dimm = spd->mod_section.unbuffered.addr_mapping >> & 0x1; >> break; > > Line too long. Please fix globally. Ok, will fix. Although, this one is only 87 characters and some of the other lines already in that file are 93 characters long. Cheers, Kyle Moffett smime.p7s Description: S/MIME cryptographic signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/7] mpc85xx: Support a board-specific processor reset routines
On Feb 21, 2011, at 15:59, Wolfgang Denk wrote:
> In message <1298311199-18775-2-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> Some board models (such as the submitted P2020-based HWW-1U-1A hardware)
>> need specialized code to run when a reset is requested to ensure proper
>> synchronization with other hardware.
>>
>> In order to facilitate such board ports, we add a board_reset_r()
>> routine which is called from the do_reset() command function instead of
>> directly poking at the MPC85xx "RSTCR" (reset control) register.
>>
>> Signed-off-by: Kyle Moffett
>> ---
>> arch/powerpc/cpu/mpc85xx/cpu.c |7 +--
>> 1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
>> index 1aad2ba..dbc662f 100644
>> --- a/arch/powerpc/cpu/mpc85xx/cpu.c
>> +++ b/arch/powerpc/cpu/mpc85xx/cpu.c
>> @@ -204,8 +204,10 @@ int checkcpu (void)
>>
>> int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> {
>> -/* Everything after the first generation of PQ3 parts has RSTCR */
>> -#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
>> +#if defined(CONFIG_BOARD_RESET_R)
>> +extern void board_reset_r(void);
>> +board_reset_r();
>> +#elif defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
>> defined(CONFIG_MPC8555) || defined(CONFIG_MPC8560)
>> unsigned long val, msr;
>
> Please implement without #ifdef's using a weak function - see
> arch/powerpc/cpu/mpc86xx/cpu.c for an example. Actually you might
> want to facto this out into common code.
>
> Side note: don't use ever "extern" function declarations in the code;
> use proper prototype declarations in some header file instead.
Hmm, ok. I thought that looked kind of funny but I copied it from somewhere
else in U-Boot so I figured it was fine.
I'll see if I can't come up with something a little more generic, otherwise
I'll use the weak function approach.
>> @@ -221,6 +223,7 @@ int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char
>> * const argv[])
>> val |= 0x7000;
>> mtspr(DBCR0,val);
>> #else
>> +/* Everything after the first generation of PQ3 parts has RSTCR */
>> volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
>
> Note that this declaration is now basicy in the middle of code, which
> is ugly at best.
>
> What is the "volatile" needed for?
It was always in the middle of code, I just moved the comment. I also didn't
add the volatile, and I have no idea why anybody thought it was needed in the
first place.
I'll fix it in v4. Thanks!
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/7] mpc85xx: Add inline GPIO acessor functions
On Feb 21, 2011, at 16:56, Wolfgang Denk wrote:
> In message you wrote:
>>
+static inline int gpio_direction_input(unsigned gpio)
+{
+ mpc85xx_gpio_set_in(1U << gpio);
+ return 0;
+}
>>>
>>> Why is this function not void when it cannot return any usefult return
>>> code anyway?
>>>
+static inline int gpio_direction_output(unsigned gpio, int value)
+{
+ mpc85xx_gpio_set_low(1U << gpio);
+ return 0;
+}
>>>
>>> Ditto.
>>
>> This is the "Linux-compatible" gpio layer that Peter Tyser was asking
>> for. I sort-of-copied most of these functions from
>> arch/nios2/include/asm/gpio.h, which just does the "return 0;" in
>> several functions.
>> Those 2 later functions are expected to be able to return an error (for
>> I2C chips and such). As I said above, if these wrappers are
>> unacceptable then I will just delete them; the only thing I use are the
>> mpc85xx_gpio_*() functions.
>
> It's not unacceptable, but maybe you add a comment to explain why
> these return int.
Ok, will fix, thanks.
>> --Apple-Mail-2-42328377
>> Content-Disposition: attachment; filename"smime.p7s"
>> Content-Type: application/pkcs7-signature; name"smime.p7s"
>> Content-Transfer-Encoding: base64
>
> Do you really have to append a 120 line binary encoded signature here?
Ah, crap, somehow S/MIME signatures got turned on. It's a tiny checkbox in the
corner of the GUI. Sorry!
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices
On Feb 21, 2011, at 16:47, Wolfgang Denk wrote:
> In message <1298311199-18775-8-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> The eXMeritus HWW-1U-1A unit is a DO-160-certified 13lb 1U chassis
>> with 3 independent TEMPEST zones. Two independent P2020 computers may
>> be found inside each zone. Complete hardware support is included.
> ...
>
> There are a number of formal issues - please run checkpatch:
> total: 12 errors, 12 warnings, 1443 lines checked
>
> ERROR: do not initialise statics to 0 or NULL
> ERROR: space prohibited after that open parenthesis '('
> ERROR: space prohibited before that close parenthesis ')'
> WARNING: Use of volatile is usually wrong: see
> Documentation/volatile-considered-harmful.txt
> WARNING: externs should be avoided in .c files
> WARNING: please, no spaces at the start of a line
>
> Please fix.
Ok, will fix. Most of the "volatile" problems were copied from the P2020DS
board and other MPC85xx boards, those are definitely wrong there too.
> ...
>> +puts("Waiting 1 sec for reset...");
>> +for (i = 0; i < 10; i++) {
>> +udelay(10);
>> +puts(".");
>> +}
>
> Do you really need a full second here?
Probably not, but this code only ever executes when a JTAG debugger has been
poking around already (the pins are always low after CPU reset), so I'd prefer
not to risk breaking it unless absolutely necessary.
> ...
>> +U_BOOT_CMD(
>> +hww1u1a_is_cpu_a, 1, 0, do_hww1u1a_is_cpu_a,
>> +"Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
>> +/* */" && echo This is CPU A\n"
>> +"if hww1u1a_is_cpu_a; then\n"
>> +"## Do stuff\n"
>> +"else\n"
>> +"## Do other stuff\n"
>> +"fi"
>
> Please don't misuse the help messages like that. Use external
> documentation instead.
Ok, is just " && echo This is CPU A\n" OK? I'm trying to show possible usage
of the command and it has no actual arguments or output on its own.
>> +#ifndef CONFIG_DDR_SPD
>> +phys_size_t fixed_sdram(void)
>> +{
>> +/* This is manual (non-SPD) DDR2 SDRAM configuration (2GB ECC) */
>> +volatile ccsr_ddr_t *ddr = (ccsr_ddr_t *)CONFIG_SYS_MPC85xx_DDR_ADDR;
>> +phys_size_t dram_size = (2048ULL) << 20;
>> +
>> +/*
>> + * First program the DDR registers, then allow time for the SDRAM
>> + * configuration and clocks to settle before enabling everything.
>> + */
>> +out_be32(&ddr->cs0_bnds,0x003F);
>> +out_be32(&ddr->cs1_bnds,0x0040007F);
>> +out_be32(&ddr->cs0_config, 0x80014202);
>> +out_be32(&ddr->cs1_config, 0x80014202);
>> +out_be32(&ddr->timing_cfg_0,0x00330804);
>> +out_be32(&ddr->timing_cfg_1,0x6f69f643);
>> +out_be32(&ddr->timing_cfg_2,0x022068cd);
>> +out_be32(&ddr->timing_cfg_3,0x0003);
>> +out_be32(&ddr->sdram_cfg_2, 0x04400010);
>> +out_be32(&ddr->sdram_mode, 0x00440452);
>> +out_be32(&ddr->sdram_mode_2,0x);
>> +out_be32(&ddr->sdram_md_cntl, 0x);
>> +out_be32(&ddr->sdram_interval, 0x0a280110);
>> +out_be32(&ddr->sdram_data_init, 0xDEADBEEF);
>> +out_be32(&ddr->sdram_clk_cntl, 0x0200);
>> +out_be32(&ddr->timing_cfg_4,0x);
>> +out_be32(&ddr->timing_cfg_5,0x);
>> +out_be32(&ddr->ddr_zq_cntl, 0x);
>> +out_be32(&ddr->ddr_wrlvl_cntl, 0x);
>> +out_be32(&ddr->ip_rev1, 0x00020403);
>> +out_be32(&ddr->ip_rev2, 0x0100);
>> +out_be32(&ddr->err_int_en, 0x000D);
>> +out_be32(&ddr->err_disable, 0x);
>> +out_be32(&ddr->err_sbe, 0x0001);
>> +out_be32(&ddr->ddr_cdr1,0x0004);
>> +out_be32(&ddr->ddr_cdr2,0x);
>> +out_be32(&ddr->sdram_cfg, 0x7300);
>> +udelay(500);
>> +out_be32(&ddr->sdram_cfg, 0xF300);
>> +
>> +/* Now wait until memory is initialized */
>> +while (in_be32(&ddr->sdram_cfg_2) & 0x0010)
>> +udelay(1000);
>
> Please avoid all these magic constants and use symbolic names instead
> like other boards are doing.
Hum.
The only difference between this code and P2020DS is that the P2020DS
development board has:
include/configs/P2020DS.h:
#define CONFIG_SYS_DDR_CS0_BNDS 0x003F
#define CONFIG_SYS_DDR_CS1_BNDS 0x
[...]
board/freescale/p2020ds/p2020ds.c:
volatile ccsr_ddr_t *ddr = (ccsr_ddr_t *)CONFIG_SYS_MPC85xx_DDR_ADDR;
ddr->cs0_config = CONFIG_SYS_DDR_CS0_CONFIG;
ddr->cs1_config = CONFIG_SYS_DDR_CS1_CONFIG;
[...]
I don't see how using a bunch of #defines with the exact same names as the CCSR
DDR registers makes the code any more readable.
I only use this code for debugging so if the numeric constants are completely
unacceptable I will just delete it rather than try to rewrite it to make
everybody happy.
> ...
>> diff --git a/include/
Re: [U-Boot] [PATCH 5/7] powerpc: Minimal private libgcc to build on Debian
On Feb 21, 2011, at 16:23, Wolfgang Denk wrote: > In message <1298311199-18775-6-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> Standard Debian powerpc and powerpcspe systems only include hard-float >> libgcc in their native compilers, which causes scary build warnings when >> building U-Boot. >> >> The easiest way to resolve this is to borrow the routines that U-Boot >> needs from the Linux kernel (GPLv2-licensed), which has the same issue. > > Actually the code says "either version 2 of the License, or (at your > option) any later version", as far as I can tell ? Ah, sorry, I dropped the "+" in there somehow. Will fix. >> Specifically, the routines are: _ashldi3(), _ashrdi3(), and _lshrdi3(). > > We have been building for years on such systems, and I don;t remember > that such issues have been reprted before for Power Architecture > systems (I remember only ARM to have such issues). Debian/RedHat used to build "nof" libraries, but they were almost completely unused and several major bugs went unnoticed for a while. Furthermore, they took a lot of time to build and they basically did not get used. Eventually it was decided to just remove the "nof" (soft-float) libgcc, etc. It's not actually fatal right now, because the 3 routines that it pulls from hard-float libgcc don't use floating point (you just get a nasty warning). The problem is that if somebody were to accidentally use one of the hard-float-based routines in U-Boot I would unexpectedly get indeterminate results instead of compile errors. > Please also follow the rules when copying code from Linux - provide > exact reference; see bullet # 4 at > http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign Will fix, thanks! Cheers, Kyle Moffett smime.p7s Description: S/MIME cryptographic signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/5] mpc85xx: Add inline GPIO acessor functions
Whoops! Please ignore this grouping of patches (1-3 of 5), I accidentally specified the wrong commit range and sent 3 emails before I realized. I've since resent the correct patch queue. My apologies! Cheers, Kyle Moffett On Sep 13, 2010, at 11:51, Kyle Moffett wrote: > To ease the implementation of other MPC85xx board ports, several common > GPIO helpers are added to . > > Since each of these compiles to no more than 4-5 instructions it would > be very inefficient to call them out of line, therefore we put them > entirely in the header file. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC][PATCH 3/3] Add board support for the eXMeritusHWW-1U-1A devices
On Sep 07, 2010, at 18:09, Peter Tyser wrote:
>>> The GPIO functions above aren't hww1u1a specific. What about adding
>>> generic 85xx GPIO functions so others can use them too?
>>
>> I can do that. Do you have any particular place you recommend I put them?
>
> The 2 places that jump to mind are drivers/gpio, or
> arch/powerpc/cpu/mpc8xxx. My personal preference would be drivers/gpio.
Hmm, given that they all seem to inline smaller than a function call would be,
I've put them as "static inline" functions in:
arch/powerpc/include/asm/mpc85xx_gpio.h
Is that OK?
>>> Is the above check the same as the hww1u1a_is_cpu_a() function
>>> previously added?
>>
>> Oops, yes it is, will fix. That reminds me of one other question,
>> though. Several variables are different between the CPU A and the CPU
>> B on the board but maintaining 2 nearly-identical device trees is kind
>> of a pain. Is there any good way to manage that? Should I try to
>> poke the device tree file during boot, or is there some kind of macro
>> language I can use in the dts file? (EG: "#ifdef CPU_A", etc)
>
> There isn't a macro language in the dts file that you can use. Its hard
> to say if you should tweak the dtb in U-Boot. People's opinions vary:
> http://www.mail-archive.com/u-boot@lists.denx.de/msg36029.html
>
> I'd give modifying the dtb in U-Boot a try, and people will let you know
> if its outside of the scope of U-Boot's responsibility.
Hmm, seems like a lot of extra work to try fiddling with the DTB for very
little gain.
I think at this point I'll just implement an "is_cpu_a" command and use it from
a hush script something like this:
if is_cpu_a; then
bootm ${flkernel} ${flinitramfs} ${fldevicetree_cpua}
else
bootm ${flkernel} ${flinitramfs} ${fldevicetree_cpub}
fi
Does that seem OK?
+/* Flash configuration registers */
+#define CONFIG_SYS_BR0_PRELIM (BR_PHYS_ADDR(FLASH0_PHYS) | BR_PS_16 |
BR_V)
+#define CONFIG_SYS_BR1_PRELIM (BR_PHYS_ADDR(FLASH1_PHYS) | BR_PS_16 |
BR_V)
+#define CONFIG_SYS_OR0_PRELIM 0xf8000ff7
+#define CONFIG_SYS_OR1_PRELIM 0xf8000ff7
>>>
>>> There are ORx defines. They should be used to make it clear what the
>>> settings above are at a glance.
>>
>> Ok, I'll have to go look up what those bits mean since I copied them
>> unmodified from the P2020DS board.
>
> If you are using different localbus devices and/or board clocking from
> the p2020ds, it'd be good to review the settings as they likely could be
> improved for your specific setup.
The default settings are the most pessimistic configuration parameters for the
onboard FLASH. As it is plenty fast for our purposes right now, I'm
disinclined to risk breaking this particular piece of code right now. I did
rewrite it so it doesn't use magic numbers anymore, but I'm not sure it makes
it much more readable :-D.
+/* PCI-E dual-port E1000 (external ethernet ports) */
+#define CONFIG_E1000 1
+#define CONFIG_E1000_FALLBACK_MAC { 0x00, 0x05, 0x93, 0x81, 0xff, 0xfe }
>>>
>>> The general U-Boot policy is to not set default MACs.
>>
>> The e1000e chips on our board need their EEPROMs to be programmed by
>> software during provisioning. At one point I think we needed this in
>> order for the programming to work successfully, but it's possible that
>> no longer applies; I'll go verify it. Regardless, this particular MAC
>> address is one I selected from our suballocation specifically for this
>> purpose.
>
> The general philosophy is that U-Boot shouldn't include default MAC
> addresses as inevitably at some point you'll have multiple boards on the
> network with identical MACs, or customers will corrupt their EEPROM and
> fall back to the default MAC, etc.
Well, due to the design of the E1000E hardware, a corrupted EEPROM (as opposed
to an unprogrammed EEPROM) usually just causes the whole E1000E chip to drop
off the PCI bus...
I'm having other functional annoyances with our programming process right now,
so I'll drop this part for now and revisit it later once I've got those
isolated.
Cheers,
Kyle Moffett
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC][PATCH 3/3] Add board support for the eXMeritus HWW-1U-1A devices
Peter, I've got one more followup question as I work through these updates: On Sep 03, 2010, at 00:00, Peter Tyser wrote: >> --- a/Makefile >> +++ b/Makefile >> @@ -2499,6 +2499,10 @@ P2020DS_36BIT_config \ >> P2020DS_config: unconfig >> @$(MKCONFIG) -t $(@:_config=) P2020DS ppc mpc85xx p2020ds freescale >> >> +HWW_1U_1A_36BIT_config \ >> +HWW_1U_1A_config:unconfig >> + @$(MKCONFIG) -t $(@:_config=) HWW_1U_1A ppc mpc85xx hww-1u-1a exmeritus >> + > > The new way to add a board is via boards.cfg. You shouldn't need to > modify this Makefile. Is there some appropriate documentation on how boards.cfg is supposed to work? The latest u-boot.git still seems to have the P2020DS lines that I referenced in "Makefile", and it has no references at all to "P2020DS" in the "boards.cfg" file. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC][PATCH 3/3] Add board support for the eXMeritusHWW-1U-1A devices
On Sep 07, 2010, at 17:40, Peter Tyser wrote: > Hi Kyle, >> The latest u-boot.git still seems to have the P2020DS lines that I >> referenced in "Makefile", and it has no references at all to "P2020DS" in >> the "boards.cfg" file. > > It looks like the top-level Makefile is still required for boards that > support multiple configs, eg 'make P2020DS_36BIT' will configure U-Boot > to compile a 36-bit addressable U-Boot image while 'make P2020DS' will > configure a 32-bit addressable U-Boot image, but they both use the same > Makefile rule with some fancy parsing. So I guess if you have a need > for both a 32 and 36bit version of U-Boot you'll to modify the Makefile > like your original patch did unfortunately. If you don't need to > support both U-Boot configs, you can go the route of other similar > boards that are only in boards.cfg, eg P4080DS, P1022DS, XPedite5370, > etc. Hmm... Well I'd like to just have my config always be 36-bit to support up to 4GB of RAM. On the other hand, I also still need to support 32-bit kernels (with only 2GB of usable RAM, obviously). Unfortunately right now I don't see any way of doing both from the same U-Boot image and device tree, as a 36-bit U-Boot maps critical I/O much higher in physical address space. This leads to another question: Is there any way to create a memory hole just like x86_64 systems do? Specifically, I'm thinking about a physical memory-map like this: 0x - 0x7fff [ 2GB] DDR2 Memory 0x8000 - 0x9fff [512MB] PCIE3 Window 0xa000 - 0xbfff [512MB] PCIE2 Window 0xc000 - 0xdfff [512MB] PCIE1 Window 0xe000 - 0xefff [256MB] FLASH (2x128MB) 0xf000 - 0x [256MB] Miscellaneous I/O resources 0x1 - 0x17fff [ 2GB] DDR2 Memory, inaccessible to 32-bit OS This is basically the same as my current 32-bit memory mapping, but with all memory above 2GB mapped separately further up in the address space. >From what I understand there would be 2 parts to the change: (1) Modifying U-Boot to have "memory hole" start and end addresses, between which it would not create TLB entries that map to RAM. (2) Modifying U-Boot and/or the kernel to split the RAM into 2 ranges in the device tree. I'm very willing to try to tackle this, but I'd need some pointers from the U-Boot community about where to start. Cheers, Kyle Moffett ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
