broadcom.
ClosedPublic
Actions

Authored by landonf on Jun 20 2016, 4:42 AM.

Details

Reviewers

mizhka
adrian

Commits

rS302190: Replace mips/sentry5 with mips/broadcom

Summary

The delta between SENTRY5 and BCM was already small due to BCM being
derived from SENTRY5; re-integrating the two avoids the maintenance
overhead of keeping them both in sync with bhnd(4) changes.

Changes:

Re-integrate minor SENTRY5 deltas in bcm_machdep.c
Modify uart_bus_chipc to allow uart(4) to detect the console uart; fixes multiple-uart handling.
Modify uart_cpu_chipc to allow specifying UART debug/console flags via kenv and device hints.
Switch SENTRY5 to std.broadcom
Enabled CFI flash support for SENTRY5

Test Plan

Tested on:

RT-N16, RT-N53
WGT634U

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4282
Build 4326: arc lint + arc unit

Event Timeline

landonf updated this revision to Diff 17704.Jun 20 2016, 4:42 AM

landonf retitled this revision from to Replace mips/sentry5 with mips/broadcom..

landonf updated this object.

landonf edited the test plan for this revision. (Show Details)

landonf added reviewers: adrian, mizhka.

Herald added a subscriber: imp. · View Herald TranscriptJun 20 2016, 4:42 AM

landonf mentioned this in D6886: Enable cfi/cfid flash support for SENTRY5 devices..Jun 20 2016, 4:42 AM

if we're shifting kernel configs around, we should follow the arm tradition of std.XXXX for their base configs, not XXX_BASE. Would you mind renaming stuff appropriately?

Super-nice change. I had a lot of thoughts about it.
I'm asking for few changes in inline comments, but please feel free to ask any question, argue, reject :)

sys/mips/broadcom/bcm_machdep.c
112–115	Please keep this debug bits, because CFE works fine not in 100% cases ;)
173–175	Could you please change "//" to #if 0 #endif for code and text comment to /* .... */?
255	"double_count" is a bit misleading variable name. What's about "clk_rate_ratio" or "clk_ratio"? Also it will be nice to have comment with examples of chips for a half and full tick counting.
sys/mips/broadcom/uart_cpu_chipc.c
98–102	Could you please keep HINTS as optional thing? I.e. if there is no hints at all, we assume UART0 as Console?
sys/mips/conf/BCM47XX
1–6 ↗	(On Diff #17704)	Could you please add 53xx in description (like 5356-5358)? I prefer to have kernel config per MIPS family, rather than BCM chip model. For example, 5358 and 4718 (74K family) are very-very close to each other in term of conf, but far from 4704 / 5350 / 5364P (aka BCM3302 or MIPS 4Kc). May be following configs: std.broadcom BCM BCM.4Kc? Also I prefer to keep Sentry untouched, because there are Sentry customers & sub-freebsd-projects which use SENTRY as siba-based. It will break their codebase and force them to give up or migrate to BHND, which still in stable stage.
21–27 ↗	(On Diff #17704)	I don't see "device pci" for BCM47xx. is it correct?
32–37 ↗	(On Diff #17704)	Could you please move SPI/CFI devices to common config?
sys/mips/conf/SENTRY5
29–30	May be it worth to remove?

This revision now requires changes to proceed.Jun 20 2016, 7:52 AM

In D6897#144684, @adrian wrote:

if we're shifting kernel configs around, we should follow the arm tradition of std.XXXX for their base configs, not XXX_BASE. Would you mind renaming stuff appropriately?

Sure!

In D6897#144704, @mizhka_gmail.com wrote:

Super-nice change. I had a lot of thoughts about it.
I'm asking for few changes in inline comments, but please feel free to ask any question, argue, reject :)

Woot. Comment replies inlined :-)

sys/mips/broadcom/bcm_machdep.c
112–115	If you're OK with it, I'll rework this to be a debug macro rather than #ifdef blocks for readability.
173–175	Sure. I was mostly using the non-standard // so it'd stand out.
255	I thought the same regarding the name (I wanted to call it clockdiv), but I was just mirroring the name of the mips_timer_init_params parameter. I'm fine changing it, though, even if it differs from mips_timer_init_params() a bit.
sys/mips/broadcom/uart_cpu_chipc.c
98–102	Sure; why don't I live the `found` case out into a function, and then call it as a fallback with a set of default uart0 values.
sys/mips/conf/BCM47XX
1–6 ↗	(On Diff #17704)	Could you please add 53xx in description (like 5356-5358)? Sure. I prefer to have kernel config per MIPS family, rather than BCM chip model. For example, 5358 and 4718 (74K family) are very-very close to each other in term of conf, but far from 4704 / 5350 / 5364P (aka BCM3302 or MIPS 4Kc). If we can get away with two generic configurations (74K, 4K) and rely on product-specific configurations in the cases where necessary, and/or modules for device drivers, I'd be pretty happy. However, I'm wary of laying claim to "BCM"/"broadcom" ; eventually we'll need to support the ARM bcma-based SoCs, and there's already an arm/broadcom containing the bcm2835 RPI code. I am ambivalent about the 47xx naming; but I'm not sure what else to call this family of bcma/siba-based MIPS SoCs. Also I prefer to keep Sentry untouched, because there are Sentry customers & sub-freebsd-projects which use SENTRY as siba-based. It will break their codebase and force them to give up or migrate to BHND, which still in stable stage. I switched SENTRY5 over to bhnd(4) once it was at feature parity with the existing code; I've been operating under the assumption that folks aren't using using the SENTRY5 code, since the port was incomplete and non-functional without updates, and most drivers (uart, pcib, usb, etc) were unimplemented or stubbed out.
32–37 ↗	(On Diff #17704)	I'd like to save that flash space if the drivers aren't required, especially on the smaller hardware.
sys/mips/conf/SENTRY5
29–30	Seems to be unused, so may as well.

landonf added a parent revision: D6896: bhnd(4) chipc child enumeration using the chipc capabilities..Jun 20 2016, 9:10 PM

landonf marked 2 inline comments as done.Jun 21 2016, 2:31 AM

landonf added inline comments.

sys/mips/broadcom/bcm_machdep.c

255

Revisiting this, I misspoke. Here's the actual usage in mips_timer_init_params:

/*
  * XXX: Some MIPS32 cores update the Count register only every two
  * pipeline cycles.
  * We know this because of status registers in CP0, make it automatic.
  */
if (double_count != 0)
	counter_freq /= 2;

Changes:

Drop unused references to sentry5's s5reg.h in mips/atheros.
Re-introduce CFE memory map debugging using a local BCM_TRACE() macro.
style(9) fixes for non-PMU reset TODO.
Fall back to uart0 if no uart hints are available.
Revert the BCM/BCM_BASE rename in favor of minimal changes to allow existing SENTRY5/BCM to both use std.broadcom.

The BCM/BCM_BASE rename wasn't necessary for the mips/sentry5 -> mips/broadcom transition, and we're probably better off tackling changes to naming/config separately.

landonf updated this object.Jun 21 2016, 4:18 AM

landonf edited edge metadata.

adrian accepted this revision.Jun 21 2016, 4:34 AM

adrian edited edge metadata.