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>