Hello Rafał,
On Fri, 28 Nov 2014, Paul Walmsley wrote:
> On Thu, 27 Nov 2014, Rafał Miłecki wrote:
>
> > I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
> > I wouldn't dare to move such a MIPS-focused driver to some common
> > place ;)
> >
> > Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
> > think you'll like it much more. Hopefully you will even consider it
> > ready for moving to the drivers/firmware/ or whatever :)
>
> OK I will take a look at this, and will either send comments, or will
> send a Reviewed-By:.
I had the chance to take a brief look at this file, and you are right: I
like your newer version better than the older one :-)
It is too bad that it seems this flash area has to be accessed very early
in boot. That certainly makes it more difficult to write something
particularly elegant. It is also a pity that it is unknown how to verify
that the flash MMIO window has been configured before reading from these
areas of the address space. But under the circumstances, calling
bcm47xx_nvram_init_from_mem() with the appropriate addresses from the bus
code during early init, as you did, seems rather reasonable. I also like
the code that you added to read the flash data from MTD later in boot.
Here are a few very minor comments that you are welcome to take or leave
as you wish.
1. In nvram_find_and_copy(), the flash header copy loop uses:
for (i = 0; i < sizeof(struct nvram_header); i += 4)
*dst++ = *src++;
Consider using either __raw_readl() to read from the MMIO window, or
possibly memcpy_fromio(). In theory, all MMIO accesses should use
functions like these.
2. In nvram_find_and_copy(), the flash payload copy loop uses:
for (; i < header->len && i < NVRAM_SPACE && i < size; i += 4)
*dst++ = le32_to_cpu(*src++);
Consider using readl() instead of the direct MMIO read & endianness
conversion.
3. In nvram_find_and_copy(), I don't believe that this is necessary:
memset(dst, 0x0, NVRAM_SPACE - i);
since nvram_buf[] is a file-static variable, and thus should have been
initialized to all zeroes.
4. As with #3 above, in nvram_init(), I don't believe that this is
necessary:
memset(dst + bytes_read, 0x0, NVRAM_SPACE - bytes_read);
5. In bcm47xx_nvram_getenv(), this multiple assignment exists:
end[0] = end[1] = '\0';
Best to avoid multiple assignments, per Chapter 1 of
Documentation/CodingStyle. You might consider running checkpatch.pl on
the file:
$ scripts/checkpatch.pl -f --strict arch/mips/bcm47xx/nvram.c
CHECK: No space is necessary after a cast
#101: FILE: arch/mips/bcm47xx/nvram.c:101:
+ src = (u32 *) header;
CHECK: No space is necessary after a cast
#102: FILE: arch/mips/bcm47xx/nvram.c:102:
+ dst = (u32 *) nvram_buf;
CHECK: multiple assignments should be avoided
#195: FILE: arch/mips/bcm47xx/nvram.c:195:
+ end[0] = end[1] = '\0';
CHECK: Alignment should match open parenthesis
#202: FILE: arch/mips/bcm47xx/nvram.c:202:
+ if ((eq - var) == strlen(name) &&
+ strncmp(var, name, (eq - var)) == 0) {
6. bcm47xx_nvram_getenv() calls strchr(). Perhaps it would be better to
use strnchr(), in case the flash data is corrupted or in an invalid
format?
7. There are a few magic numbers in this code, mostly in
bcm47xx_nvram_gpio_pin(). It might be worth converting those to macros
and documenting the expectations there in a comment above the macro.
8. The way that bcm47xx_nvram_gpio_pin() calls bcm47xx_nvram_getenv()
seems a bit inefficient. It might be better to loop over all of the keys
in the shadowed flash, looking for values that match "name". Then if the
key name matches "gpio" plus one or two digits, the code could just return
the digits. That way, only one pass is needed, rather than 32 (in the
worst case). Well, at least the reads should be coming from cached DRAM,
rather than flash...
If you fix/address those (or correct my review ;-) ), then you're
welcome to add my Reviewed-by: to a patch that moves this file out to
drivers/firmware.
regards,
- Paul
