[BHND] ChipCommon: Resource managers, Serial & Parallel Flash support,

[mizhka]

Summary

This patch introduced support for:

• ChipCommon resource managers - to manage resource assignment & allocation for sub-drivers
• Serial Flash - via SPI (for instance, mx25l)
• Parallel flash - via CFI

Minor improvements:

• parsing of capabilities
• print noprobes
• flash slicer (for Asus TRX firmware)

Test Plan

BUS - tested on Asus RT-N53
Serial Flash - tested on Asus RT-N53 (mx25l)
Parallel flash - tested on Asus RT-N16

[landon_landonf.org]

I like the broken out caps structure.

I've inlined an initial set of comments; I still need to look at the resource/port/region handling in more detail.

Rather than highlight the style(9) issues again, the style feedback in D6249 and D6247 largely apply here; I've only added inline comments on issues not previously mentioned.

sys/dev/bhnd/cores/chipc/chipc.c

180 ↗

(On Diff #15986)

For clarity/safety, either use explicit != 0, or drop the > 0 entirely in your return value checks; otherwise, negative return values appear to be an intentional success case.

186 ↗

(On Diff #15986)

style(9): /* Most single-line comments look like this. */

209 ↗

(On Diff #15986)

The uart count should either always be treated as signed, or as unsigned, rather than converting between the two.

217 ↗

(On Diff #15986)

s/gener_/generic_/

sys/dev/bhnd/cores/chipc/chipc.h

53–58 ↗

(On Diff #15986)

Do these need to be externally visible?

Also, s/capacilities/capabilities/

sys/dev/bhnd/cores/chipc/chipc_slicer.c

106 ↗

(On Diff #15986)

You can use the bus_(read|write)_* macros with res directly, rather than having to save a local copy of the tag/handle.

sys/dev/bhnd/cores/chipc/chipcvar.h

85 ↗

(On Diff #15986)

Minor style(9) issue:

New style code should prefer C99 uintXX_t types to u_intXX_t. Since these are just boolean flags, you may as well use bool here.

[mizhka]

• fixed style(9) issues
• aligned type of num_uarts variable (uint8_t)
• other cosmetic changes

[mizhka]

• resolve merge conflicts after today's rebase

[landon_landonf.org]

Inlined a number of bus-related comments; apologies, I still haven't had time to look at the chipcbus resource handling in enough detail.

sys/dev/bhnd/cores/chipc/chipc.c

224 ↗

(On Diff #16059)

Since you're parsing out the capability flags, you may as well drop caps from the softc and simply read the value to a local variable for parsing.

225 ↗

(On Diff #16059)

ChipCommon should implement the bus_* methods directly, rather than having a chipcbus intermediary that will need access to the ChipCommon's driver's internals and bhnd_* bus APIs anyway.

231 ↗

(On Diff #16059)

This inverts the usual newbus behavior of using DEVICE_PROBE/DEVICE_IDENTIFY to probe and attach drivers for bus devices.

I lean towards DEVICE_IDENTIFY, as that'd isolate ChipCommon from what may be SoC-specific drivers, but using DEVICE_PROBE with a BUS_PROBE_NOWILDCARD return value would also work.

If you then instead expose the chipc_capabilities structure via bhnd_chipc_if.m (and some mechanism for requesting the flash config from chipc), you can implement this driver selection through the standard newbus methods by inspecting the capability flags in device_probe and/or device_identify for these drivers.

sys/dev/bhnd/cores/chipc/chipc_slicer.c

72 ↗

(On Diff #16059)

device_get_class() can be used with direct pointer comparison against the result of devclass_find("mx25l")

However, I'd suggest just defining driver-specific entry points to chipc_slicer_walk() and registering the appropriate one when you first introspect the ChipCommon flash configuration.

[mizhka]

• update due to merge conflicts with recent trunk changes. no functional changes. tested on board

[adrian]

sys/dev/bhnd/cores/chipc/chipc.c

282 ↗

(On Diff #16527)

missing space before "

[mizhka]

• reduced code changes thanks to recent commits
• remove dependence on flash_if.h

[landon_landonf.org]

Looks good, just inlined two minor comments.

sys/dev/bhnd/cores/chipc/bhnd_chipc_if.m

103–111 ↗

(On Diff #16952)

Could we add these flash configuration options to the chipc_caps structure? I was thinking we could use a union {} over a set of flash-type-specific structures, and then let chipc abstract the actual fetching/parsing of flash configuration.

sys/dev/bhnd/cores/chipc/chipc_cfi.c

75 ↗

(On Diff #16952)

Since driver loading/unloading can result in DEVICE_IDENTIFY being called multiple times, drivers have to manually check for an existing device node to prevent registering a duplicate.

(see DEVICE_IDENTIFY(4) for an example)

[mizhka]

• add check for previous results of device_identify

[landon_landonf.org]

Minor issue related to the module build when testing against BCM4331.

sys/dev/bhnd/cores/chipc/bhnd_chipc_if.m

42 ↗

(On Diff #17006)

This is triggering a linker error in module builds due to the bhnd(4) module providing the bhnd_chipc_if interface, but bhnd_chipc_generic_get_caps() being defined in chipc.

Since the bhnd_chipc interface is meant to define an explicit target device's interface (e.g. the intention is to implement this interface for the EXTIF core on earlier pre-ChipCommon devices), and not a bus interface delegated to parent device, I think it's safe to not provide a _generic_ implementation here, and ideal to not delegate to the parent device.

[mizhka]

sys/dev/bhnd/cores/chipc/bhnd_chipc_if.m

42 ↗

(On Diff #17006)

The purpose of method is to provide capabilities to all sub-sub-sub-devices. For instance, ascii-based NVRAM is stored on flash, flash is attached to ChipCommon, ChipCommon has information about NVRAM type (OTP or SPROM or ..)

As compromise, is it OK to put default method directly in bhnd_chipc_if?

[landon_landonf.org]

sys/dev/bhnd/cores/chipc/bhnd_chipc_if.m

42 ↗

(On Diff #17006)

Generally kobj bus methods that walk the newbus tree include child and parent parameters, which allows the parent that handles the method to determine the identity of the requesting child.

If you don't mind me tweaking these APIs a bit later (once I can test the results on a local device), that compromise sounds fine to me.

[mizhka]

sys/dev/bhnd/cores/chipc/bhnd_chipc_if.m

42 ↗

(On Diff #17006)

Landon, could you please move comment to D6584?
BTW, i'll update it soon.

[adrian]

yay!