Driver from Daisuke Aoyama for review.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Overall it's good. We just need to make it more FreeBSD source code friendly.
Major style comments:
- A lot of empty lines;
- A lot of indentation problems in function calls that span multiple lines, like device_printf();
- Check your code for the 78/80 column limit.
How about defining a function to fill out the message header/tag?
Instead of doing:
#ifdef DEBUG
printf(...)
#endif
Please define a DPRINTF macro.
Maybe we don't need a new header file and you can put everything in bcm2835_mbox.h?
| sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c | ||
|---|---|---|
| 73 | Is this still relevant? It looks like we switch to a semaphore... | |
| 74 | You could indent the backslashes in these macros. | |
| 152 | This is an error and should not be under if (bootverbose). | |
| 171 | Idem. This is an error. | |
| 178 | Might be a good idea to use a separate variable to store an uint8_t * cast of msg instead of casting multiple times. | |
| 186 | An error again. | |
| 290 | This is not properly indented as per style(9). | |
| 309 | This is not properly indented as per style(9). | |
| 347 | This is not properly indented as per style(9). | |
| 366 | This is not properly indented as per style(9). | |
| 406 | This is not properly indented as per style(9). | |
| 464 | This is not properly indented as per style(9). | |
| 520 | This is not properly indented as per style(9). | |
| 533 | You have too many parenthesis here. | |
| 534 | As per style(9), the '?' should be on the line above. | |
| 578 | This is not properly indented as per style(9). | |
| 847 | The "g_" prefix to signify a global variable isn't really used on FreeBSD. It's up to you, though... | |
| 913 | Is this a known problem? Have you investigated why? | |
| 952 | Do we really need a delay() here? | |
| 989 | Why delay() if we just return afterwards? | |
| 1025 | Same problem? This is repeated in every sysctl handler. | |
| 1056 | This is not properly indented as per style(9). | |
| 1153 | I'm not sure this handling of write-only sysctl is correct. Why not follow the regular pattern avoiding the if (!req->newptr) above? | |
| 1261 | This is wide: over 80 columns. | |
| 1271 | Hmm, maybe it would be best if we could put these in the softc? | |
| 1290 | The '=' should be in the previous line, as per style(9). | |
| 1306 | This should probably be under bootverbose. | |
| 1544 | This is using spaces instead of tabs. | |
| 1547 | Spaces instead of tabs. | |
| 1628 | This is not properly indented as per style(9). | |
| 1639 | The '&&' should go in the line above. | |
| 1700 | Too many empty lines in this function. | |
Comment Actions
I tried to build this code but it fails with:
cc -c -O -pipe -std=c99 -g -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Winline -Wcast-qual -Wundef -Wno-pointer-sign -fformat-extensions -Wmissing-include-dirs -fdiagnostics-show-option -Wno-error-tautological-compare -Wno-error-empty-body -Wno-error-parentheses-equality -Wno-error-unused-function -nostdinc -I. -I/data/rpi/head/src/sys -I/data/rpi/head/src/sys/contrib/altq -I/data/rpi/head/src/sys/contrib/libfdt -I/data/rpi/head/src/sys/gnu/dts/include -D_KERNEL -DHAVE_KERNEL_OPTION_HEADERS -include opt_global.h -funwind-tables -ffreestanding -gdwarf-2 -mfpu=none -mllvm -arm-enable-ehabi -Werror /data/rpi/head/src/sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c
/data/rpi/head/src/sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c:161:19: error: use of undeclared identifier 'BCM2835_MBOX_CHAN_PROP'
MBOX_WRITE(mbox, BCM2835_MBOX_CHAN_PROP, (uint32_t)sc->dma_phys);
^/data/rpi/head/src/sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c:162:18: error: use of undeclared identifier 'BCM2835_MBOX_CHAN_PROP'
MBOX_READ(mbox, BCM2835_MBOX_CHAN_PROP, &err);
^2 errors generated.
| sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c | ||
|---|---|---|
| 1778 | We usually remove #if 0 code... Either it shouldn't be commented out or it is useless... :) | |
Comment Actions
The other thing that bothers me about this driver is that typically frequency and voltage are scaled together... first voltage is increased, then frequency, to scale up. When scaling back, first frequency is reduced, then voltage. I see one line of code in the cpufreq set routine that lowers voltage, nothing that ever increases it except manual controls by the user.
| sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c | ||
|---|---|---|
| 258 | There appears to be no reason to have all these similar lines that clear the response bit in the length right after setting all the bits of the length to a value known to be far less than 0x80000000. | |
| 898 | This should say why it's writing it twice. If it's a workaround and we're not sure why it's necessary, it's okay to say that. | |
| 1426 | The identify and probe stuff seems wrong, fdt-based systems shouldn't need identify() methods. The fdt bindings docs say that the root node should have a "brcm,bcm2835" compatible property, can we key off of that in the probe routine? (May need to add another DRIVER_MODULE() allowing attachment to ofwbus until we get the ofwbus/simplebus inheritence stuff into the tree.) | |
| 1448 | the softc is pre-zeroed by newbus before being given to the driver (before probe is called, so values set in probe can be used later in attach if necessary) | |
| 1487 | The flags should be zero here. NOWAIT is for use when you're in a context where you can't sleep for memory allocation, such as when holding a non-sleeping lock or in an interrupt filter handler. (The reality is this is never going to fail or sleep anyway.) | |
| sys/arm/broadcom/bcm2835/bcm2835_mbox_prop.h | ||
| 82 | all of these message structures that have a req/resp union seem like good candidates for an anonymous union (just leave out the name "body" in both the definition and all the dereferences). | |
| sys/arm/broadcom/bcm2835/bcm2835_ofwcpu.c | ||
| 229 | I think this entire file does not belong here. The cpulist code in powerpc/ofw/ofw_cpu.c probably needs to move into dev/ofw so it can be used by all platforms. Also, it should be noted that the published bindings for bcm2385 don't even include a cpus node. The current (incorrect) freebsd dts file does, but if we rely on this it will just make it harder to some day move to using vendor-supplied .dtb files. | |
| sys/arm/broadcom/bcm2835/bcm2835_ofwcpu.c | ||
|---|---|---|
| 229 | We could create our own DTS with just this cpu node and then merge it (/include/) with the vendor-supplied DTS when building a kernel. | |
| sys/arm/broadcom/bcm2835/bcm2835_cpufreq.c | ||
|---|---|---|
| 1426 | Hmm, ignore the previous comment about how this driver attaches. I didn't understand the requirements of the cpufreq device interface. It looks like cpufreq is kind of poorly designed for the requirements of modern arm chips, but I guess we need to work with it as-is for now. | |
Comment Actions
Relying one comment fro Aoyama-san:
About voltage control:
All voltages are controled by VC firmware.
If you don't set force_turbo=1 in config.txt, it causes implicitly maximum voltage
specified in config.txt when you call "set clock" other than 700000000 (default clock).
However, the clock lower than 700000000 should work well with default voltage,
so the driver reset to default voltage from max or any values if the freq lower than
min_freq specified in config.txt.
Anonymous union:
The union body exist for calculation of total size of the buffer.
msg->tag_hdr.val_buf_size = sizeof(msg->body);