[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

61 ↗

(On Diff #15985)

Missing space after closing ) and before {.

73 ↗

(On Diff #15985)

Missing space after closing ) and before {.

87 ↗

(On Diff #15985)

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.

89 ↗

(On Diff #15985)

Missing space before ( and after ).

96 ↗

(On Diff #15985)

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.

97 ↗

(On Diff #15985)

Missing space before ( and after ).

102 ↗

(On Diff #15985)

Missing space before ( and after ).

103 ↗

(On Diff #15985)

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

107 ↗

(On Diff #15985)

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

110 ↗

(On Diff #15985)

Missing space before (.

123 ↗

(On Diff #15985)

I think this goes beyond 80 characters.

[sobomax]

Few more nits.

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

29 ↗

(On Diff #15985)

Missing:

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

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

28 ↗

(On Diff #15985)

Missing:

+ * $FreeBSD$
  */

35 ↗

(On Diff #15985)

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

[sobomax]

Few more minor fixes needed.

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

29 ↗

(On Diff #15992)

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>

78 ↗

(On Diff #15992)

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

81 ↗

(On Diff #15992)

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

108 ↗

(On Diff #15992)

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 ↗

(On Diff #16029)

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

static int
mipscore_probe(device_t dev)
{

64 ↗

(On Diff #16029)

Extra blank line.

75 ↗

(On Diff #16029)

See a note above for mipscore_probe.

83 ↗

(On Diff #16029)

Move uint16_t devid declaration up two lines.

90 ↗

(On Diff #16029)

Move int error declaration together with other vars.

91 ↗

(On Diff #16029)

Another space is missing before (.

96 ↗

(On Diff #16029)

Another space is missing before (.

101 ↗

(On Diff #16029)

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 ↗

(On Diff #16029)

@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