Page MenuHomeFreeBSD

[BHND SoC] Enable interrupts for Broadcom MIPS processor
ClosedPublic

Authored by adrian on May 6 2016, 5:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 26, 8:44 PM
Unknown Object (File)
Fri, Apr 26, 8:44 PM
Unknown Object (File)
Fri, Apr 26, 8:44 PM
Unknown Object (File)
Fri, Apr 26, 8:44 PM
Unknown Object (File)
Fri, Apr 26, 8:44 PM
Unknown Object (File)
Fri, Apr 26, 7:11 PM
Unknown Object (File)
Fri, Apr 26, 7:08 PM
Unknown Object (File)
Fri, Apr 26, 7:07 PM
Subscribers

Details

Reviewers
sobomax
mizhka
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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mizhka retitled this revision from to [BHND SoC] Enable interrupts for Broadcom MIPS processor.
mizhka updated this object.
mizhka edited the test plan for this revision. (Show Details)
mizhka added reviewers: adrian, landon_landonf.org.
mizhka set the repository for this revision to rS FreeBSD src repository - subversion.
mizhka added a subscriber: sobomax.
sobomax requested changes to this revision.May 6 2016, 6:55 PM
sobomax added a reviewer: 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

Missing space after closing ) and before {.

73

Missing space after closing ) and before {.

87

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

Missing space before ( and after ).

96

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

Missing space before ( and after ).

102

Missing space before ( and after ).

103

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

107

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

110

Missing space before (.

123

I think this goes beyond 80 characters.

This revision now requires changes to proceed.May 6 2016, 6:55 PM

Few more nits.

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

Missing:

#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
sys/dev/bhnd/cores/mips/mipscorevar.h
28

Missing:

+ * $FreeBSD$
  */
35

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

mizhka edited edge metadata.
mizhka marked 13 inline comments as done.
mizhka edited edge metadata.

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 edited edge metadata.

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).

This revision now requires changes to proceed.May 6 2016, 9:48 PM
mizhka edited edge metadata.
  • Correct $FreeBSD$
  • Use bus_space_read_region to read MIPS core registers

Bunch more style(9) suggestions.

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

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

static int
mipscore_probe(device_t dev)
{
65

Extra blank line.

76

See a note above for mipscore_probe.

84

Move uint16_t devid declaration up two lines.

91

Move int error declaration together with other vars.

92

Another space is missing before (.

97

Another space is missing before (.

102

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.
104

@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.

mizhka edited edge metadata.
  • remove malloc call and read of registers
  • fixed style(9) issues
landon_landonf.org edited edge metadata.

Should this live in sys/mips/broadcom?

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

mizhka edited edge metadata.
  • renamed sys/dev/bhnd/cores/mips/* to sys/mips/broadcom/*
adrian edited edge metadata.

damnit, committed it but missed the attribution :(

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

adrian edited reviewers, added: mizhka; removed: adrian.

Committed!

mizhka edited edge metadata.

Already integrated; I believe this can be abandoned?

sobomax edited edge metadata.

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

This revision is now accepted and ready to land.Jul 18 2016, 5:11 AM