Hi Florian, Please find my inline comments. Regards, Tatha
-----Original Message----- From: Florian Fainelli [mailto:f.faine...@gmail.com] On Behalf Of Florian Fainelli Sent: 28 February 2012 14:54 To: Tathagata Das Cc: 'OpenWrt Development List'; 'Hauke Mehrtens' Subject: Re: [PATCH] Updated kernel patch in trunk to support brcm47xx BCMA NAND flash Hello Tatha, Le 02/28/12 07:50, Tathagata Das a écrit : > Hi, > Attached is the updated kernel patch to support brcm47xx BCMA NAND flash. > I have used latest trunk source code to create this patch. I have tested it > on Netgear WNR3500Lv2. > > Thanks to Florian and Hauke for their comments. And thank you for your efforts on this, I am sorry, but I found a couple of small issues in your patch, which I attached inline for easier commenting: 9991-bcm47xx-bcma-nandflash.patch diff -Naur a/arch/mips/bcm47xx/bus.c b/arch/mips/bcm47xx/bus.c --- a/arch/mips/bcm47xx/bus.c 2012-02-17 16:34:21.000000000 +0530 +++ b/arch/mips/bcm47xx/bus.c 2012-02-23 18:22:17.000000000 +0530 @@ -2,6 +2,7 @@ * BCM947xx nvram variable access * * Copyright (C) 2011 Hauke Mehrtens<ha...@hauke-m.de> + * Copyright (C) 2011-2012 Tathagata Das<tathag...@alumnux.com> * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -92,3 +93,9 @@ sflash->numblocks = scc->sflash.numblocks; sflash->size = scc->sflash.size; } + +void bcm47xx_nflash_struct_bcma_init(struct bcm47xx_nflash *nflash, struct bcma_drv_cc *bcc) +{ + nflash->nflash_type = BCM47XX_BUS_TYPE_BCMA; + nflash->bcc = bcc; +} diff -Naur a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c --- a/arch/mips/bcm47xx/nvram.c 2012-02-17 16:34:22.000000000 +0530 +++ b/arch/mips/bcm47xx/nvram.c 2012-02-23 18:20:57.000000000 +0530 @@ -4,6 +4,7 @@ * Copyright (C) 2005 Broadcom Corporation * Copyright (C) 2006 Felix Fietkau<n...@openwrt.org> * Copyright (C) 2010-2011 Hauke Mehrtens<ha...@hauke-m.de> + * Copyright (C) 2011-2012 Tathagata Das<tathag...@alumnux.com> * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -21,6 +22,7 @@ #include<asm/mach-bcm47xx/nvram.h> #include<asm/mach-bcm47xx/bcm47xx.h> #include<asm/mach-bcm47xx/bus.h> +#include<linux/mtd/bcm47xx_nand.h> char nvram_buf[NVRAM_SPACE]; EXPORT_SYMBOL(nvram_buf); @@ -159,6 +161,53 @@ return 0; } +static int early_nvram_init_nflash(void) +{ + struct nvram_header *header; + u32 off; + int ret; + int len; + u32 flash_size = bcm47xx_nflash.size; + u8 tmpbuf[NFL_SECTOR_SIZE]; + int i; + u32 *src, *dst; + + /* check if the struct is already initilized */ + if (!flash_size) + return -1; + + cfe_env = 0; + + off = FLASH_MIN; + while (off<= flash_size) { + ret = bcma_nflash_read(bcm47xx_nflash.bcc, off, NFL_SECTOR_SIZE, tmpbuf); + if (ret != NFL_SECTOR_SIZE) { + goto done; + } The brackets here are not required. [Tatha]: Done. + header = (struct nvram_header *)tmpbuf; + if (header->magic == NVRAM_HEADER) { + goto found; + } They are not required here too. [Tatha]: Done. + off<<= 1; + } + + ret = -1; + goto done; + +found: + len = header->len; + header = (struct nvram_header *) KSEG1ADDR(NAND_FLASH1 + off); + src = (u32 *) header; + dst = (u32 *) nvram_buf; + for (i = 0; i< sizeof(struct nvram_header); i += 4) + *dst++ = *src++; + for (; i< len&& i< NVRAM_SPACE; i += 4) + *dst++ = *src++; Did you have to do this because memcpy() did not work, due to byte access instead of word access? [Tatha]: Yes, you are right. + ret = 0; +done: + return ret; +} + [snip] + +/* Issue a nand flash command */ +static inline void bcma_nflash_cmd(struct bcma_drv_cc *cc, u32 opcode) +{ + bcma_cc_write32(cc, NAND_CMD_START, opcode); + bcma_cc_read32(cc, NAND_CMD_START); +} + +/* Check offset and length */ +static int bcma_nflash_offset_is_valid(struct bcma_drv_cc *cc, u32 offset, u32 len, u32 mask) +{ + if ((offset& mask) != 0 || (len& mask) != 0) { + pr_err("%s(): Address is not aligned. offset: %x, len: %x, mask: %x\n", __func__, offset, len, mask); + BUG_ON(1); + return 1; + } a BUG_ON() here is a little hard, just the printk should be sufficient in case someone is doing unaligned accesses. [Tatha]: Done. + + if ((((offset + len)>> 20)> cc->nflash.size) || + ((((offset + len)>> 20) == cc->nflash.size)&& these two lines are equal to: ((offset + len)>> 20)>= cc->nflash.size [Tatha]: Done. + (((offset + len)& ((1<< 20) - 1)) != 0))) { + pr_err("%s(): Address is outside Flash memory region. offset: %x, len: %x, mask: %x\n", __func__, offset, len, mask); + return 1; + } + + return 0; +} Everything else is fine from my perspective otherwise. Thank you. -- Florian _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
