[BHND SoC] Enable interrupts for Broadcom MIPS processor

[adrian]

Summary

Broadcom MIPS processor doesn't reset TI flag if additional manipulation is done

Thanks to @sobomax!

Test Plan

Tested on ASUS RT-N16 & RT-N53

[sobomax]

Functionally looks good. Needs some style(9) cleanup, though. Please make sure to re-read that manual page.

sys/dev/bhnd/cores/mips/mipscore.c

62

Missing space after closing ) and before {.

74

Missing space after closing ) and before {.

88

struct resource* res -> struct resource *res

I also thing style(9) discourages variable allocation on stack in the body of fuction. You might want to move "struct resource* res" declaration up.

90

Missing space before ( and after ).

97

I think this goes beyond 80 characters. I also thing style(9) discourages variable allocation on stack in the body of fuction. You might want to move "struct mipscore_regs *regs" declaration up.

98

Missing space before ( and after ).

103

Missing space before ( and after ).

104

I think this goes beyond 80 characters. Also missing space after two commas.

108

I think this goes beyond 80 characters. Also missing space after two commas.

111

Missing space before (.

124

I think this goes beyond 80 characters.

[sobomax]

Few more nits.

sys/dev/bhnd/cores/mips/mipscore.c

30

Missing:

#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");

sys/dev/bhnd/cores/mips/mipscorevar.h

29

Missing:

+ * $FreeBSD$
  */

36

Not sure what does "*<" in the comment mean :)

[sobomax]

Few more minor fixes needed.

sys/dev/bhnd/cores/mips/mipscore.c

30

Nope, this should be the following before your first #include, unless you are in the ".h" file:

#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");

You already have cdefs.h, so just do the following and remove $FreeBSD from the comment here.

  #include <sys/cdefs.h>
+ __FBSDID("$FreeBSD$");
+ 
  #include <sys/param.h>

79

I'd add just to make sure that later on if some changes are made to the code you won't get with the bogus pointer in the cleanup:

          struct mipscore_regs *regs;

+         regs = NULL;

Not a critical code path (only run once per boot), so it won't hurt anything.

[landon_landonf.org]

Sweet. I just had some minor comments, in addition to the style(9) stuff.

sys/dev/bhnd/cores/mips/mipscore.c

82

The type promotion won't break anything, but for consistency, bhnd device IDs are unsigned 16-bit values (uint16_t).

109

Is the _multi_ variant correct here? (It repeatedly reads count elements from the same location in bus space).

[mizhka]

• Correct $FreeBSD$
• Use bus_space_read_region to read MIPS core registers

[sobomax]

Bunch more style(9) suggestions.

sys/dev/bhnd/cores/mips/mipscore.c

63

Missing \n before mipscore_probe and another one between ) and {, should be

static int
mipscore_probe(device_t dev)
{

64

Extra blank line.

75

See a note above for mipscore_probe.

83

Move uint16_t devid declaration up two lines.

90

Move int error declaration together with other vars.

91

Another space is missing before (.

96

Another space is missing before (.

101

Variable definition should probably go to the beginning of the function and separated from the initialization:

From style(9):

Be careful to not obfuscate the code by initializing variables in the
declarations.  Use this feature only thoughtfully.  DO NOT use function
calls in initializers.

103

@mizhka_gmail.com speaking of this I am not 100% sure about merits of M_NOWAIT ms. M_WAITOK here. Conceptually I'd rather wait for a while and attach, than run without core driver with CPU in broken state (e.g. that TI interrupt issue). That being said, I also understand that in practice it would be hard to construct a plausible case for that to fail. Maybe check what other low-level drivers do to avoid spreading bad code around via copy-pasto.

[adrian]

space!

[mizhka]

• remove malloc call and read of registers
• fixed style(9) issues

[mizhka]

• fix compilation issue

[landon_landonf.org]

Should this live in sys/mips/broadcom?

Otherwise, if @sobomax and @adrian are fine with this, this works for me.

[mizhka]

• renamed sys/dev/bhnd/cores/mips/* to sys/mips/broadcom/*

[adrian]

damnit, committed it but missed the attribution :(

[mizhka]

Committed.
I don't see any way to close it except "abandon". sorry

[adrian]

Committed!

[landon_landonf.org]

Already integrated; I believe this can be abandoned?

[sobomax]

This has been integrated already. GC this for good, so it does not stick in the middle of my list. :)

[sobomax]

BB