Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

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