Page MenuHomeFreeBSD
Differential D6250

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

Authored by mizhka on May 6 2016, 5:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 24, 11:24 PM
Unknown Object (File)
Mon, Mar 17, 4:41 PM
Unknown Object (File)
Sat, Mar 8, 5:35 AM
Unknown Object (File)
Fri, Mar 7, 6:47 AM
Unknown Object (File)
Thu, Mar 6, 7:24 PM
Unknown Object (File)
Feb 23 2025, 5:39 AM
Unknown Object (File)
Feb 21 2025, 8:04 AM
Unknown Object (File)
Feb 18 2025, 2:05 PM
View All Files
Subscribers
imp

Details

Reviewers
landonf
adrian
Commits
rS301409: bhnd(4): Add support for chipc-attached flash
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

Diff Detail

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

Event Timeline

mizhka updated this revision to Diff 15986.May 6 2016, 5:35 PM
mizhka retitled this revision from to [BHND] ChipCommon: BUS, UART, Serial & parallel Flash support.
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.
Herald added a subscriber: imp. · View Herald TranscriptMay 6 2016, 5:35 PM
landon_landonf.org edited edge metadata.May 6 2016, 10:43 PM

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
179

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.

185

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

208

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

216

s/gener_/generic_/

sys/dev/bhnd/cores/chipc/chipc.h
53–60

Do these need to be externally visible?

Also, s/capacilities/capabilities/

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

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

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.

landon_landonf.org requested changes to this revision.May 6 2016, 10:43 PM
landon_landonf.org edited edge metadata.
This revision now requires changes to proceed.May 6 2016, 10:43 PM
mizhka updated this revision to Diff 16046.May 9 2016, 10:34 AM
mizhka edited edge metadata.
mizhka marked an inline comment as done.
  • fixed style(9) issues
  • aligned type of num_uarts variable (uint8_t)
  • other cosmetic changes
mizhka updated this revision to Diff 16059.May 9 2016, 3:53 PM
mizhka edited edge metadata.
  • resolve merge conflicts after today's rebase
landon_landonf.org requested changes to this revision.May 12 2016, 11:19 PM
landon_landonf.org edited edge metadata.

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
178

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.

179

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.

185

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
73

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.

This revision now requires changes to proceed.May 12 2016, 11:19 PM
mizhka updated this revision to Diff 16411.May 16 2016, 9:59 PM
mizhka retitled this revision from [BHND] ChipCommon: BUS, UART, Serial & parallel Flash support to [BHND] ChipCommon: Resource managers, Serial & Parallel Flash support, .
mizhka updated this object.
mizhka edited the test plan for this revision. (Show Details)
mizhka edited edge metadata.
mizhka marked 3 inline comments as done.
mizhka marked 5 inline comments as done.
mizhka added a parent revision: D6400: [GEOM/disk] Add function to find disk by device softc.May 16 2016, 10:16 PM
mizhka updated this revision to Diff 16527.May 18 2016, 3:01 PM
mizhka edited edge metadata.
  • update due to merge conflicts with recent trunk changes. no functional changes. tested on board
adrian accepted this revision.May 19 2016, 9:17 PM
adrian edited edge metadata.
adrian added inline comments.
sys/dev/bhnd/cores/chipc/chipc.c
181

missing space before "

landon_landonf.org mentioned this in D6471: Implement pass-through resource management for ChipCommon.May 20 2016, 2:55 AM
landon_landonf.org mentioned this in D6525: Integrate bhnd_chipc's BUS_ADD_CHILD() with a child resource mapping table..May 24 2016, 5:38 AM
adrian mentioned this in rS300702: [bhnd] Integrate bhnd_chipc's BUS_ADD_CHILD() with a child resource mapping….May 26 2016, 12:43 AM
mizhka updated this revision to Diff 16952.May 27 2016, 1:01 AM
mizhka edited edge metadata.
  • reduced code changes thanks to recent commits
  • remove dependence on flash_if.h
landon_landonf.org requested changes to this revision.May 27 2016, 3:44 PM
landon_landonf.org edited edge metadata.

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
76

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)

This revision now requires changes to proceed.May 27 2016, 3:44 PM
mizhka updated this revision to Diff 17006.May 27 2016, 6:00 PM
mizhka edited edge metadata.
  • add check for previous results of device_identify
landon_landonf.org removed a parent revision: D6400: [GEOM/disk] Add function to find disk by device softc.May 27 2016, 6:14 PM
landon_landonf.org added a parent revision: D6584: [BHND] (fix) set of corrections.May 27 2016, 6:17 PM
landon_landonf.org requested changes to this revision.May 27 2016, 6:27 PM
landon_landonf.org edited edge metadata.

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.

This revision now requires changes to proceed.May 27 2016, 6:27 PM
mizhka added inline comments.May 27 2016, 8:29 PM
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 added inline comments.May 27 2016, 11:29 PM
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 added inline comments.May 30 2016, 7:27 AM
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.

landon_landonf.org accepted this revision.May 31 2016, 5:41 PM
landon_landonf.org edited edge metadata.
This revision is now accepted and ready to land.May 31 2016, 5:41 PM
landonf added a child revision: D6712: Migrate SENTRY5 to bhnd(4) and unify with BCM.Jun 3 2016, 11:00 PM
landonf edited reviewers, added: landonf; removed: landon_landonf.org.Jun 4 2016, 7:01 PM
This revision now requires review to proceed.Jun 4 2016, 7:01 PM
landonf accepted this revision.Jun 4 2016, 7:01 PM
landonf edited edge metadata.
This revision is now accepted and ready to land.Jun 4 2016, 7:01 PM
adrian accepted this revision.Jun 4 2016, 7:10 PM
adrian edited edge metadata.

yay!

Closed by commit rS301409: bhnd(4): Add support for chipc-attached flash (authored by landonf). · Explain WhyJun 4 2016, 7:39 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
dev/
bhnd/
cores/
chipc/
11 lines
58 lines
88 lines
38 lines
159 lines
100 lines
214 lines
271 lines
65 lines
431 lines
74 lines
28 lines

Diff 16046

View Options

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

  • This file was added.
#include <atf-c.h>
#include <alias.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
static int randcmp(const void *a, const void *b) {
int res, r = rand();
kpUnsubmitted
Done
Inline Actions

I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this triggers a bug it may end up being nondeterministic (and thus difficult to debug).

kp: I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this…
(void)a;
(void)b;
res = (r/4 < RAND_MAX/9) ? 1
: (r/5 < RAND_MAX/9) ? 0
: -1;
return (res);
}
ATF_TC(destroynull);
ATF_TC_HEAD(destroynull, env)
{
atf_tc_set_md_var(env, "descr", "Destroy the NULL instance");
}
ATF_TC_BODY(destroynull, dummy)
{
atf_tc_expect_signal(SIGSEGV, "Code expects valid pointer.");
LibAliasUninit(NULL);
}
ATF_TC(singleinit);
ATF_TC_HEAD(singleinit, env)
{
atf_tc_set_md_var(env, "descr", "Create an instance");
}
ATF_TC_BODY(singleinit, dummy)
{
struct libalias *la;
la = LibAliasInit(NULL);
ATF_CHECK_MSG(la != NULL, "Creating an instance failed.");
LibAliasUninit(la);
}
ATF_TC(multiinit);
ATF_TC_HEAD(multiinit, env)
{
atf_tc_set_md_var(env, "descr", "Recreate an instance multiple times");
}
ATF_TC_BODY(multiinit, dummy)
{
struct libalias *la;
int i;
la = LibAliasInit(NULL);
for(i = 1; i < 30; i++) {
struct libalias *lo = la;
la = LibAliasInit(la);
ATF_CHECK_MSG(la == lo, "Recreating moved the instance around: %d", i);
}
LibAliasUninit(la);
}
ATF_TC(multiinstance);
ATF_TC_HEAD(multiinstance, env)
{
atf_tc_set_md_var(env, "descr", "Create and destoy multiple instances.");
}
ATF_TC_BODY(multiinstance, dummy)
{
struct libalias *la[300];
int const num_instances = sizeof(la) / sizeof(*la);
int i;
for (i = 0; i < num_instances; i++) {
la[i] = LibAliasInit(NULL);
ATF_CHECK_MSG(la[i] != NULL, "Creating instance %d failed.", i);
}
qsort(la, num_instances, sizeof(*la), randcmp);
for (i = 0; i < num_instances; i++)
LibAliasUninit(la[i]);
}
ATF_TP_ADD_TCS(aliasdb)
{
/* Use "dd if=/dev/random bs=2 count=1 | od -x" to reproduce */
srand(0x5ac4);
ATF_TP_ADD_TC(aliasdb, destroynull);
ATF_TP_ADD_TC(aliasdb, singleinit);
ATF_TP_ADD_TC(aliasdb, multiinit);
ATF_TP_ADD_TC(aliasdb, multiinstance);
return atf_no_error();
}
View Options

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

  • This file was added.
#include <atf-c.h>
#include <alias.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
static int randcmp(const void *a, const void *b) {
int res, r = rand();
kpUnsubmitted
Done
Inline Actions

I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this triggers a bug it may end up being nondeterministic (and thus difficult to debug).

kp: I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this…
(void)a;
(void)b;
res = (r/4 < RAND_MAX/9) ? 1
: (r/5 < RAND_MAX/9) ? 0
: -1;
return (res);
}
ATF_TC(destroynull);
ATF_TC_HEAD(destroynull, env)
{
atf_tc_set_md_var(env, "descr", "Destroy the NULL instance");
}
ATF_TC_BODY(destroynull, dummy)
{
atf_tc_expect_signal(SIGSEGV, "Code expects valid pointer.");
LibAliasUninit(NULL);
}
ATF_TC(singleinit);
ATF_TC_HEAD(singleinit, env)
{
atf_tc_set_md_var(env, "descr", "Create an instance");
}
ATF_TC_BODY(singleinit, dummy)
{
struct libalias *la;
la = LibAliasInit(NULL);
ATF_CHECK_MSG(la != NULL, "Creating an instance failed.");
LibAliasUninit(la);
}
ATF_TC(multiinit);
ATF_TC_HEAD(multiinit, env)
{
atf_tc_set_md_var(env, "descr", "Recreate an instance multiple times");
}
ATF_TC_BODY(multiinit, dummy)
{
struct libalias *la;
int i;
la = LibAliasInit(NULL);
for(i = 1; i < 30; i++) {
struct libalias *lo = la;
la = LibAliasInit(la);
ATF_CHECK_MSG(la == lo, "Recreating moved the instance around: %d", i);
}
LibAliasUninit(la);
}
ATF_TC(multiinstance);
ATF_TC_HEAD(multiinstance, env)
{
atf_tc_set_md_var(env, "descr", "Create and destoy multiple instances.");
}
ATF_TC_BODY(multiinstance, dummy)
{
struct libalias *la[300];
int const num_instances = sizeof(la) / sizeof(*la);
int i;
for (i = 0; i < num_instances; i++) {
la[i] = LibAliasInit(NULL);
ATF_CHECK_MSG(la[i] != NULL, "Creating instance %d failed.", i);
}
qsort(la, num_instances, sizeof(*la), randcmp);
for (i = 0; i < num_instances; i++)
LibAliasUninit(la[i]);
}
ATF_TP_ADD_TCS(aliasdb)
{
/* Use "dd if=/dev/random bs=2 count=1 | od -x" to reproduce */
srand(0x5ac4);
ATF_TP_ADD_TC(aliasdb, destroynull);
ATF_TP_ADD_TC(aliasdb, singleinit);
ATF_TP_ADD_TC(aliasdb, multiinit);
ATF_TP_ADD_TC(aliasdb, multiinstance);
return atf_no_error();
}
View Options

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

  • This file was added.
#include <atf-c.h>
#include <alias.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
static int randcmp(const void *a, const void *b) {
int res, r = rand();
kpUnsubmitted
Done
Inline Actions

I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this triggers a bug it may end up being nondeterministic (and thus difficult to debug).

kp: I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this…
(void)a;
(void)b;
res = (r/4 < RAND_MAX/9) ? 1
: (r/5 < RAND_MAX/9) ? 0
: -1;
return (res);
}
ATF_TC(destroynull);
ATF_TC_HEAD(destroynull, env)
{
atf_tc_set_md_var(env, "descr", "Destroy the NULL instance");
}
ATF_TC_BODY(destroynull, dummy)
{
atf_tc_expect_signal(SIGSEGV, "Code expects valid pointer.");
LibAliasUninit(NULL);
}
ATF_TC(singleinit);
ATF_TC_HEAD(singleinit, env)
{
atf_tc_set_md_var(env, "descr", "Create an instance");
}
ATF_TC_BODY(singleinit, dummy)
{
struct libalias *la;
la = LibAliasInit(NULL);
ATF_CHECK_MSG(la != NULL, "Creating an instance failed.");
LibAliasUninit(la);
}
ATF_TC(multiinit);
ATF_TC_HEAD(multiinit, env)
{
atf_tc_set_md_var(env, "descr", "Recreate an instance multiple times");
}
ATF_TC_BODY(multiinit, dummy)
{
struct libalias *la;
int i;
la = LibAliasInit(NULL);
for(i = 1; i < 30; i++) {
struct libalias *lo = la;
la = LibAliasInit(la);
ATF_CHECK_MSG(la == lo, "Recreating moved the instance around: %d", i);
}
LibAliasUninit(la);
}
ATF_TC(multiinstance);
ATF_TC_HEAD(multiinstance, env)
{
atf_tc_set_md_var(env, "descr", "Create and destoy multiple instances.");
}
ATF_TC_BODY(multiinstance, dummy)
{
struct libalias *la[300];
int const num_instances = sizeof(la) / sizeof(*la);
int i;
for (i = 0; i < num_instances; i++) {
la[i] = LibAliasInit(NULL);
ATF_CHECK_MSG(la[i] != NULL, "Creating instance %d failed.", i);
}
qsort(la, num_instances, sizeof(*la), randcmp);
for (i = 0; i < num_instances; i++)
LibAliasUninit(la[i]);
}
ATF_TP_ADD_TCS(aliasdb)
{
/* Use "dd if=/dev/random bs=2 count=1 | od -x" to reproduce */
srand(0x5ac4);
ATF_TP_ADD_TC(aliasdb, destroynull);
ATF_TP_ADD_TC(aliasdb, singleinit);
ATF_TP_ADD_TC(aliasdb, multiinit);
ATF_TP_ADD_TC(aliasdb, multiinstance);
return atf_no_error();
}
View Options

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

  • This file was added.
#include <atf-c.h>
#include <alias.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
static int randcmp(const void *a, const void *b) {
int res, r = rand();
kpUnsubmitted
Done
Inline Actions

I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this triggers a bug it may end up being nondeterministic (and thus difficult to debug).

kp: I wonder if it's worth printing the random seed somewhere, or even using a fixed seed. If this…
(void)a;
(void)b;
res = (r/4 < RAND_MAX/9) ? 1
: (r/5 < RAND_MAX/9) ? 0
: -1;
return (res);
}
ATF_TC(destroynull);
ATF_TC_HEAD(destroynull, env)
{
atf_tc_set_md_var(env, "descr", "Destroy the NULL instance");
}
ATF_TC_BODY(destroynull, dummy)
{
atf_tc_expect_signal(SIGSEGV, "Code expects valid pointer.");
LibAliasUninit(NULL);
}
ATF_TC(singleinit);
ATF_TC_HEAD(singleinit, env)
{
atf_tc_set_md_var(env, "descr", "Create an instance");
}
ATF_TC_BODY(singleinit, dummy)
{
struct libalias *la;
la = LibAliasInit(NULL);
ATF_CHECK_MSG(la != NULL, "Creating an instance failed.");
LibAliasUninit(la);
}
ATF_TC(multiinit);
ATF_TC_HEAD(multiinit, env)
{
atf_tc_set_md_var(env, "descr", "Recreate an instance multiple times");
}
ATF_TC_BODY(multiinit, dummy)
{
struct libalias *la;
int i;
la = LibAliasInit(NULL);
for(i = 1; i < 30; i++) {
struct libalias *lo = la;
la = LibAliasInit(la);
ATF_CHECK_MSG(la == lo, "Recreating moved the instance around: %d", i);
}
LibAliasUninit(la);
}
ATF_TC(multiinstance);
ATF_TC_HEAD(multiinstance, env)
{
atf_tc_set_md_var(env, "descr", "Create and destoy multiple instances.");
}
ATF_TC_BODY(multiinstance, dummy)
{
struct libalias *la[300];
int const num_instances = sizeof(la) / sizeof(*la);
int i;
for (i = 0; i < num_instances; i++) {
la[i] = LibAliasInit(NULL);
ATF_CHECK_MSG(la[i] != NULL, "Creating instance %d failed.", i);
}
qsort(la, num_instances, sizeof(*la), randcmp);
for (i = 0; i < num_instances; i++)
LibAliasUninit(la[i]);
}
ATF_TP_ADD_TCS(aliasdb)
{
/* Use "dd if=/dev/random bs=2 count=1 | od -x" to reproduce */
srand(0x5ac4);
ATF_TP_ADD_TC(aliasdb, destroynull);
ATF_TP_ADD_TC(aliasdb, singleinit);
ATF_TP_ADD_TC(aliasdb, multiinit);
ATF_TP_ADD_TC(aliasdb, multiinstance);
return atf_no_error();
}
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...