Date: Wed, 08 Aug 2007 09:39:20 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: krassi@bulinfo.net Cc: freebsd-arm@FreeBSD.ORG, ticso@cicely.de Subject: Re: CENTIPAD boot Message-ID: <20070808.093920.-432838389.imp@bsdimp.com> In-Reply-To: <46B9DD23.70608@bulinfo.net> References: <46B9CAD8.4040103@bulinfo.net> <20070808144152.GM41893@cicely12.cicely.de> <46B9DD23.70608@bulinfo.net>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <46B9DD23.70608@bulinfo.net> Krassimir Slavchev <krassi@bulinfo.net> writes: : -----BEGIN PGP SIGNED MESSAGE----- : Hash: SHA1 : : Bernd Walter wrote: : > On Wed, Aug 08, 2007 at 04:53:28PM +0300, Krassimir Slavchev wrote: : > Yes : > : > M. Warner Losh wrote: : >>>> can you resend them as a unified diff? : >>>> : >>>> Warner : >>>> : > : >> Mmm - there are points, which look at least questionable. : >> And there are a few, which are likely not related to support another : >> board. : > : : Index: boot0spi/main.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/boot0spi/main.c,v : retrieving revision 1.5 : diff -u -r1.5 main.c : - --- boot0spi/main.c 20 Dec 2006 17:50:02 -0000 1.5 : +++ boot0spi/main.c 8 Aug 2007 13:49:19 -0000 : @@ -47,10 +47,10 @@ : continue; : // Need extra copy at addr3 : memcpy(addr3, addr, (len + FLASH_PAGE_SIZE - 1) / FLASH_PAGE_SIZE * : FLASH_PAGE_SIZE); : - - printf("Writing %u bytes to flash at %u\n", len, OFFSET); : + printf("Writing %u bytes to flash at %u\n", len, LOADER_OFFSET); : for (i = 0; i < len; i+= FLASH_PAGE_SIZE) { : for (j = 0; j < 10; j++) { : - - off = i + OFFSET; : + off = i + LOADER_OFFSET; : SPI_WriteFlash(off, addr + i, FLASH_PAGE_SIZE); : SPI_ReadFlash(off, addr2 + i, FLASH_PAGE_SIZE); : if (p_memcmp(addr3 + i, addr2 + i, FLASH_PAGE_SIZE) == 0) : : > This is unrelated, but important. Agreed. : Index: bootspi/loader_prompt.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/bootspi/loader_prompt.c,v : retrieving revision 1.4 : diff -u -r1.4 loader_prompt.c : - --- bootspi/loader_prompt.c 15 Mar 2007 03:31:48 -0000 1.4 : +++ bootspi/loader_prompt.c 8 Aug 2007 13:49:21 -0000 : @@ -29,7 +29,6 @@ : #include "env_vars.h" : #include "lib.h" : #include "spi_flash.h" : - -#include "ee.h" : : /******************************* GLOBALS : *************************************/ : : @@ -286,8 +285,9 @@ : { : char buf[25]; : printf("Testing Config EEPROM\n"); : - - EEWrite(0, "This is a test", 15); : - - EERead(0, buf, 15); : + strcpy(buf,"This is a test!"); : + WriteEEPROM(0, buf, 15); : + ReadEEPROM(0, buf, 15); : printf("Found '%s'\n", buf); : break; : } : : > Why remove ee.h and then access the eeprom? : > I never used the eeprom code, so I'm unshure about it. : > At least the following line should be added befor printing: : > buf[15] = '\0'; : : WriteEEPROM() and ReadEEPROM() functions are in libat91. May be ee.c : should be removed too. : Yes, The '!' char should be removed from the string. There are two eeprom routines. One for 16-bit and one for 8-bit. That's not so good, but is kind of a pita to cope. I was planning on removing the read/write to the eeprom entirely from loader_prompt. : Index: bootspi/main.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/bootspi/main.c,v : retrieving revision 1.3 : diff -u -r1.3 main.c : - --- bootspi/main.c 21 Oct 2006 22:44:26 -0000 1.3 : +++ bootspi/main.c 8 Aug 2007 13:49:21 -0000 : @@ -41,18 +41,19 @@ : #include "emac.h" : #include "lib.h" : #include "spi_flash.h" : - -#include "ee.h" : +#include "sd-card.h" : : int : main(void) : { : printf("\nBoot\n"); : - - EEInit(); : + InitEEPROM(); : SPI_InitFlash(); : #ifdef TSC_FPGA : fpga_load(); : #endif : EMAC_Init(); : + sdcard_init(); : LoadBootCommands(); : if (getc(1) == -1) { : start_wdog(30); : : > The same as above - remove ee.h and then access the eeprom? : > I have the sdcard_init in my local changes as well. : > Although it might be better to just init the GPIO, because that's : > what we really want in case of spi booting. : > I typically need this to do a network boot and then access the SD : > card, which requires something like this. : : InitEEPROM() is in libat91 too. We should likely go to a unified foo_board.c : Index: libat91/Makefile : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/libat91/Makefile,v : retrieving revision 1.9 : diff -u -r1.9 Makefile : - --- libat91/Makefile 13 Jul 2007 14:27:04 -0000 1.9 : +++ libat91/Makefile 8 Aug 2007 13:49:22 -0000 : @@ -8,7 +8,7 @@ : putchar.c printf.c reset.c spi_flash.c xmodem.c \ : sd-card.c strcvt.c strlen.c strcmp.c memcpy.c strcpy.c \ : memset.c memcmp.c : - -SRCS+=ashldi3.c divsi3.c : +SRCS+=ashldi3.c divsi3.S : NO_MAN= : : .if ${MK_TAG_LIST} != "no" : : > Why is the filename change needed? : > This is obviously unrelated to the board support. : > Do we have a Makefile error in CVS? : : I can't find divsi3.c in the source tree. Yes it seems to be Makefile error. I think I need to checkin divsi3.c. It is *MUCH* smaller code, even if it isn't optimal. : Index: libat91/arm_init.S : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/libat91/arm_init.S,v : retrieving revision 1.2 : diff -u -r1.2 arm_init.S : - --- libat91/arm_init.S 20 Dec 2006 18:16:49 -0000 1.2 : +++ libat91/arm_init.S 8 Aug 2007 13:49:22 -0000 : @@ -61,7 +61,7 @@ : #ifdef BOOT_IIC : .long (TWI_EEPROM_SIZE >> 9) : #else : - -#ifdef BOOT_BWCT : +#if defined(BOOT_BWCT) | defined(BOOT_CENTIPAD) : .long ((528 << 17) | (13 << 13) | (12 * 2)) : #else : .long ((1056 << 17) | (13 << 13) | (12 * 2)) : : > In the long run we should start defining those things to align with the : > SPI flash type and then just setup the type related to the board. : : Yes, I agree. Yes. We can actually look at the ID and know what the parameters are. However, we'd need to put a 'generic' image into the part and 'fix' it later. : Index: libat91/eeprom.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/libat91/eeprom.c,v : retrieving revision 1.3 : diff -u -r1.3 eeprom.c : - --- libat91/eeprom.c 20 Dec 2006 18:19:52 -0000 1.3 : +++ libat91/eeprom.c 8 Aug 2007 13:49:23 -0000 : @@ -33,7 +33,11 @@ : : /* Use a macro to calculate the TWI clock generator value to save code : space. */ : #define AT91C_TWSI_CLOCK 100000 : - -#define TWSI_EEPROM_ADDRESS 0x50 : +#ifdef BOOT_CENTIPAD : +#define TWSI_EEPROM_ADDRESS 0x57 : +#else : +#define TWSI_EEPROM_ADDRESS 0x50 : +#endif : : #define TWI_CLK_BASE_DIV ((AT91C_MASTER_CLOCK/(4*AT91C_TWSI_CLOCK)) - 2) : #define SET_TWI_CLOCK ((0x00010000) | (TWI_CLK_BASE_DIV) | : (TWI_CLK_BASE_DIV << 8)) : Index: libat91/emac.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/libat91/emac.c,v : retrieving revision 1.8 : diff -u -r1.8 emac.c : - --- libat91/emac.c 13 Jul 2007 14:27:04 -0000 1.8 : +++ libat91/emac.c 8 Aug 2007 13:49:25 -0000 : @@ -321,7 +321,7 @@ : if (serverPort != udpHdr->src_port) : break; : : - - TFTP_ACK_Data(tftpHdr->data, : + TFTP_ACK_Data((char *)tftpHdr->data, : SWAP16(tftpHdr->block_num), : SWAP16(udpHdr->udp_len) - 12); : } : : > This chunk seems to be unrelated to the board type as well. : Yes, this was reported before but still uncommitted. I need to merge that in. : @@ -339,9 +339,9 @@ : */ : #ifndef BOOT_BWCT : static unsigned short : - -AT91F_MII_ReadPhy (AT91PS_EMAC pEmac, unsigned char addr) : +AT91F_MII_ReadPhy (AT91PS_EMAC pEmac, unsigned char phyaddr, unsigned : char addr) : { : - - unsigned value = 0x60020000 | (addr << 18); : + unsigned value = 0x60020000 | ((phyaddr & 0x1f) << 23) | (addr << 18); : : pEmac->EMAC_CTL |= AT91C_EMAC_MPE; : pEmac->EMAC_MAN = value; : @@ -359,9 +359,9 @@ : */ : #ifdef BOOT_TSC : static unsigned short : - -AT91F_MII_WritePhy (AT91PS_EMAC pEmac, unsigned char addr, unsigned : short s) : +AT91F_MII_WritePhy (AT91PS_EMAC pEmac, unsigned char phyaddr, unsigned : char addr, unsigned short s) : { : - - unsigned value = 0x50020000 | (addr << 18) | s; : + unsigned value = 0x50020000 | ((phyaddr & 0x1f) << 23) | (addr << 18) | s; : : pEmac->EMAC_CTL |= AT91C_EMAC_MPE; : pEmac->EMAC_MAN = value; : @@ -380,6 +380,7 @@ : static void : MII_GetLinkSpeed(AT91PS_EMAC pEmac) : { : + unsigned char phyaddr = 0; : #if defined(BOOT_TSC) || defined(BOOT_KB920X) || defined(BOOT_CENTIPAD) : unsigned short stat2; : #endif : @@ -388,14 +389,18 @@ : unsigned sec; : int i; : #endif : - -#ifdef BOOT_BWCT : +#ifdef BOOT_CENTIPAD : + phyaddr = 0x10; : +#endif : + : +#if defined(BOOT_BWCT) | defined(BOOT_CENTIPAD) : /* hardcoded link speed since we connect a switch via MII */ : update = pEmac->EMAC_CFG & ~(AT91C_EMAC_SPD | AT91C_EMAC_FD); : update |= AT91C_EMAC_SPD; : update |= AT91C_EMAC_FD; : #endif : #if defined(BOOT_KB920X) || defined(BOOT_CENTIPAD) : - - stat2 = AT91F_MII_ReadPhy(pEmac, MII_STS2_REG); : + stat2 = AT91F_MII_ReadPhy(pEmac, phyaddr, MII_STS2_REG); : if (!(stat2 & MII_STS2_LINK)) : return ; : update = pEmac->EMAC_CFG & ~(AT91C_EMAC_SPD | AT91C_EMAC_FD); : @@ -407,7 +412,7 @@ : #ifdef BOOT_TSC : while (1) { : for (i = 0; i < 10; i++) { : - - stat2 = AT91F_MII_ReadPhy(pEmac, MII_STS_REG); : + stat2 = AT91F_MII_ReadPhy(pEmac, phyaddr, MII_STS_REG); : if (stat2 & MII_STS_LINK_STAT) : break; : printf("."); : @@ -418,11 +423,11 @@ : if (stat2 & MII_STS_LINK_STAT) : break; : printf("Resetting MII..."); : - - AT91F_MII_WritePhy(pEmac, 0x0, 0x8000); : - - while (AT91F_MII_ReadPhy(pEmac, 0x0) & 0x8000) continue; : + AT91F_MII_WritePhy(pEmac, phyaddr, 0x0, 0x8000); : + while (AT91F_MII_ReadPhy(pEmac, phyaddr, 0x0) & 0x8000) continue; : } : printf("emac: link"); : - - stat2 = AT91F_MII_ReadPhy(pEmac, MII_SPEC_STS_REG); : + stat2 = AT91F_MII_ReadPhy(pEmac, phyaddr, MII_SPEC_STS_REG); : update = pEmac->EMAC_CFG & ~(AT91C_EMAC_SPD | AT91C_EMAC_FD); : if (stat2 & (MII_SSTS_100FDX | MII_SSTS_100HDX)) { : printf(" 100TX"); : : > Are you shure, that you want to nail the link speed too 100/full? : > It is only reasonable if you have a switch, but then it wouldn't make : > sense to set a hardcoded phyaddr for your board, since almost every : > switch I know uses multiple phy-addresses. : > On the other hand - I like the phyaddr change, because it will allow : > me to setup my switch from loader some day and not from kernel, as I do : > now. : : Yes, I should do the link negotiation. Link negotiation is important here. : Index: libat91/emac_init.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/libat91/emac_init.c,v : retrieving revision 1.4 : diff -u -r1.4 emac_init.c : - --- libat91/emac_init.c 20 Dec 2006 18:26:37 -0000 1.4 : +++ libat91/emac_init.c 8 Aug 2007 13:49:26 -0000 : @@ -94,7 +94,7 @@ : AT91C_PA8_ETXEN | AT91C_PA16_EMDIO | AT91C_PA9_ETX0 | : AT91C_PA10_ETX1 | AT91C_PA11_ECRS_ECRSDV | AT91C_PA15_EMDC | : AT91C_PA7_ETXCK_EREFCK; : - -#if defined(BOOT_KB920X) | defined(BOOT_BWCT) /* Really !RMII */ : +#if defined(BOOT_KB920X) | defined(BOOT_BWCT) | defined(BOOT_CENTIPAD) : /* Really !RMII */ : AT91C_BASE_PIOB->PIO_BSR = : AT91C_PB12_ETX2 | AT91C_PB13_ETX3 | AT91C_PB14_ETXER | : AT91C_PB15_ERX2 | AT91C_PB16_ERX3 | AT91C_PB17_ERXDV | : Index: libat91/spi_flash.c : =================================================================== : RCS file: /home/ncvs/src/sys/boot/arm/at91/libat91/spi_flash.c,v : retrieving revision 1.4 : diff -u -r1.4 spi_flash.c : - --- libat91/spi_flash.c 28 Mar 2007 22:38:01 -0000 1.4 : +++ libat91/spi_flash.c 8 Aug 2007 13:49:26 -0000 : @@ -119,7 +119,7 @@ : byteAddress = flash_addr % FLASH_PAGE_SIZE; : : p_memset(tx_commandBuffer, 0, 8); : - -#ifdef BOOT_BWCT : +#if defined(BOOT_BWCT) | defined(BOOT_CENTIPAD) : tx_commandBuffer[0] = 0xd2; : tx_commandBuffer[1] = ((pageAddress >> 6) & 0xFF); : tx_commandBuffer[2] = ((pageAddress << 2) & 0xFC) | : @@ -177,7 +177,7 @@ : byteAddress = flash_addr % FLASH_PAGE_SIZE; : : p_memset(tx_commandBuffer, 0, 8); : - -#ifdef BOOT_BWCT : +#if defined(BOOT_BWCT) | defined(BOOT_CENTIPAD) : tx_commandBuffer[0] = 0x82; : tx_commandBuffer[1] = ((pageAddress >> 6) & 0xFF); : tx_commandBuffer[2] = ((pageAddress << 2) & 0xFC) | : @@ -256,7 +256,7 @@ : value = pSPI->SPI_RDR; : value = pSPI->SPI_SR; : : - -#ifdef BOOT_BWCT : +#if defined(BOOT_BWCT) | defined(BOOT_CENTIPAD) : if (((value = GetFlashStatus()) & 0xFC) != 0xB4) : printf(" Bad SPI status: 0x%x\n", value); : #else : : > Although this is correct with our current code. : > Please split BOOT_BWCT and BOOT_CENTIPAD here, since I localy have : > 0xAC added as a valid status: : > #ifdef BOOT_BWCT : > value = GetFlashStatus(); : > if ((value & 0xFC) != 0xAC : > && (value & 0xFC) != 0xB4) : > printf(" Bad SPI status: 0x%x\n", value); : > #else : > This is because I use AT45DB161D chips in production. : > The 0xB4 is from the AT45DB321C, which I'd used in prototypes only. : > I might use other SPI types for special purspose as well. : : Feel free to change anything you wish! I think we need just a little more smarts here. What this check is doing is making sure that the ID is sane, which doesn't make sense for a general solution. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070808.093920.-432838389.imp>