| Message ID | 20200421041655.82856-6-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add sbefifo backend | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (6ae2ba655ca5e24b403a33bf15dff7261d3e7052) |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
On Tuesday, 21 April 2020 2:16:41 PM AEST Amitay Isaacs wrote: > When PDBG_BACKEND_DTB is specified, backend may not be set. This means > only the drivers registered with PDBG_DEFAULT_BACKEND will be loaded. > To be able to match backend specific drivers for targets in system tree, > use the backend specified in PDBG_BACKEND_DRIVER environment variable. > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > libpdbg/hwunit.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/libpdbg/hwunit.c b/libpdbg/hwunit.c > index 074ddef..3d6a05d 100644 > --- a/libpdbg/hwunit.c > +++ b/libpdbg/hwunit.c > @@ -14,6 +14,7 @@ > * limitations under the License. > */ > > +#include <stdlib.h> > #include <string.h> > #include <assert.h> > > @@ -70,12 +71,44 @@ static const struct hw_unit_info *find_compatible(enum > pdbg_backend backend, return NULL; > } > > +static enum pdbg_backend get_backend_driver(void) > +{ > + const char *tmp; > + enum pdbg_backend backend = PDBG_DEFAULT_BACKEND; > + > + tmp = getenv("PDBG_BACKEND_DRIVER"); > + if (tmp) { > + if (!strcmp(tmp, "fsi")) > + backend = PDBG_BACKEND_FSI; > + else if (!strcmp(tmp, "i2c")) > + backend = PDBG_BACKEND_I2C; > + else if (!strcmp(tmp, "kernel")) > + backend = PDBG_BACKEND_KERNEL; > + else if (!strcmp(tmp, "fake")) > + backend = PDBG_BACKEND_FAKE; > + else if (!strcmp(tmp, "host")) > + backend = PDBG_BACKEND_HOST; > + else if (!strcmp(tmp, "cronus")) > + backend = PDBG_BACKEND_CRONUS; > + } > + > + return backend; > +} > + > const struct hw_unit_info *pdbg_hwunit_find_compatible(const char > *compat_list, uint32_t len) > { > - const struct hw_unit_info *p; > + const struct hw_unit_info *p = NULL; > + enum pdbg_backend backend = pdbg_get_backend(); > + > + if (backend == PDBG_DEFAULT_BACKEND) { > + backend = get_backend_driver(); The flow here was initially a little confusing for me to figure out what was going on here and why, although I understand now - you hit this case when an external backend DTB is specified and you need to figure out the driver collection to use. That makes sense but I think this code belongs with the rest of the initialisation code in libpdbg/dtb.c. Can we initialise the backend correctly with the rest of the environment variables in pdbg_default_dtb() such that pdbg_get_backend() just returns the "correct" thing? - Alistair > + if (backend != PDBG_DEFAULT_BACKEND) > + p = find_compatible(backend, compat_list, len); > + } else { > + p = find_compatible(backend, compat_list, len); > + } > > - p = find_compatible(pdbg_get_backend(), compat_list, len); > if (!p) > p = find_compatible(PDBG_DEFAULT_BACKEND, compat_list, len);
On Thu, 2020-04-30 at 10:07 +1000, Alistair Popple wrote: > On Tuesday, 21 April 2020 2:16:41 PM AEST Amitay Isaacs wrote: > > When PDBG_BACKEND_DTB is specified, backend may not be set. This > > means > > only the drivers registered with PDBG_DEFAULT_BACKEND will be > > loaded. > > To be able to match backend specific drivers for targets in system > > tree, > > use the backend specified in PDBG_BACKEND_DRIVER environment > > variable. > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > --- > > libpdbg/hwunit.c | 37 +++++++++++++++++++++++++++++++++++-- > > 1 file changed, 35 insertions(+), 2 deletions(-) > > > > diff --git a/libpdbg/hwunit.c b/libpdbg/hwunit.c > > index 074ddef..3d6a05d 100644 > > --- a/libpdbg/hwunit.c > > +++ b/libpdbg/hwunit.c > > @@ -14,6 +14,7 @@ > > * limitations under the License. > > */ > > > > +#include <stdlib.h> > > #include <string.h> > > #include <assert.h> > > > > @@ -70,12 +71,44 @@ static const struct hw_unit_info > > *find_compatible(enum > > pdbg_backend backend, return NULL; > > } > > > > +static enum pdbg_backend get_backend_driver(void) > > +{ > > + const char *tmp; > > + enum pdbg_backend backend = PDBG_DEFAULT_BACKEND; > > + > > + tmp = getenv("PDBG_BACKEND_DRIVER"); > > + if (tmp) { > > + if (!strcmp(tmp, "fsi")) > > + backend = PDBG_BACKEND_FSI; > > + else if (!strcmp(tmp, "i2c")) > > + backend = PDBG_BACKEND_I2C; > > + else if (!strcmp(tmp, "kernel")) > > + backend = PDBG_BACKEND_KERNEL; > > + else if (!strcmp(tmp, "fake")) > > + backend = PDBG_BACKEND_FAKE; > > + else if (!strcmp(tmp, "host")) > > + backend = PDBG_BACKEND_HOST; > > + else if (!strcmp(tmp, "cronus")) > > + backend = PDBG_BACKEND_CRONUS; > > + } > > + > > + return backend; > > +} > > + > > const struct hw_unit_info *pdbg_hwunit_find_compatible(const char > > *compat_list, uint32_t len) > > { > > - const struct hw_unit_info *p; > > + const struct hw_unit_info *p = NULL; > > + enum pdbg_backend backend = pdbg_get_backend(); > > + > > + if (backend == PDBG_DEFAULT_BACKEND) { > > + backend = get_backend_driver(); > > The flow here was initially a little confusing for me to figure out > what was > going on here and why, although I understand now - you hit this case > when an > external backend DTB is specified and you need to figure out the > driver > collection to use. > > That makes sense but I think this code belongs with the rest of the > initialisation code in libpdbg/dtb.c. Can we initialise the backend > correctly > with the rest of the environment variables in pdbg_default_dtb() such > that > pdbg_get_backend() just returns the "correct" thing? Sure. I was also not happy about having to do separate checks elsewhere in the code. Are you ok with the environment variable PDBG_BACKEND_DRIVER? I thought of using PDBG_BACKEND earlier, but we are specifying the backend device tree, so that seemed odd to also specify PDBG_BACKEND. That's why PDBG_BACKEND_DRIVER, which is backend setting for selecting a collection of drivers. Let me know what you prefer. > > - Alistair > > > + if (backend != PDBG_DEFAULT_BACKEND) > > + p = find_compatible(backend, compat_list, len); > > + } else { > > + p = find_compatible(backend, compat_list, len); > > + } > > > > - p = find_compatible(pdbg_get_backend(), compat_list, len); > > if (!p) > > p = find_compatible(PDBG_DEFAULT_BACKEND, compat_list, > > len); > > > Amitay.
On Thursday, 30 April 2020 11:22:22 AM AEST Amitay Isaacs wrote: > On Thu, 2020-04-30 at 10:07 +1000, Alistair Popple wrote: > > On Tuesday, 21 April 2020 2:16:41 PM AEST Amitay Isaacs wrote: > > > When PDBG_BACKEND_DTB is specified, backend may not be set. This > > > means > > > only the drivers registered with PDBG_DEFAULT_BACKEND will be > > > loaded. > > > To be able to match backend specific drivers for targets in system > > > tree, > > > use the backend specified in PDBG_BACKEND_DRIVER environment > > > variable. > > > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > > --- > > > > > > libpdbg/hwunit.c | 37 +++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 35 insertions(+), 2 deletions(-) > > > > > > diff --git a/libpdbg/hwunit.c b/libpdbg/hwunit.c > > > index 074ddef..3d6a05d 100644 > > > --- a/libpdbg/hwunit.c > > > +++ b/libpdbg/hwunit.c > > > @@ -14,6 +14,7 @@ > > > > > > * limitations under the License. > > > */ > > > > > > +#include <stdlib.h> > > > > > > #include <string.h> > > > #include <assert.h> > > > > > > @@ -70,12 +71,44 @@ static const struct hw_unit_info > > > *find_compatible(enum > > > pdbg_backend backend, return NULL; > > > > > > } > > > > > > +static enum pdbg_backend get_backend_driver(void) > > > +{ > > > + const char *tmp; > > > + enum pdbg_backend backend = PDBG_DEFAULT_BACKEND; > > > + > > > + tmp = getenv("PDBG_BACKEND_DRIVER"); > > > + if (tmp) { > > > + if (!strcmp(tmp, "fsi")) > > > + backend = PDBG_BACKEND_FSI; > > > + else if (!strcmp(tmp, "i2c")) > > > + backend = PDBG_BACKEND_I2C; > > > + else if (!strcmp(tmp, "kernel")) > > > + backend = PDBG_BACKEND_KERNEL; > > > + else if (!strcmp(tmp, "fake")) > > > + backend = PDBG_BACKEND_FAKE; > > > + else if (!strcmp(tmp, "host")) > > > + backend = PDBG_BACKEND_HOST; > > > + else if (!strcmp(tmp, "cronus")) > > > + backend = PDBG_BACKEND_CRONUS; > > > + } > > > + > > > + return backend; > > > +} > > > + > > > > > > const struct hw_unit_info *pdbg_hwunit_find_compatible(const char > > > > > > *compat_list, uint32_t len) > > > > > > { > > > > > > - const struct hw_unit_info *p; > > > + const struct hw_unit_info *p = NULL; > > > + enum pdbg_backend backend = pdbg_get_backend(); > > > + > > > + if (backend == PDBG_DEFAULT_BACKEND) { > > > + backend = get_backend_driver(); > > > > The flow here was initially a little confusing for me to figure out > > what was > > going on here and why, although I understand now - you hit this case > > when an > > external backend DTB is specified and you need to figure out the > > driver > > collection to use. > > > > That makes sense but I think this code belongs with the rest of the > > initialisation code in libpdbg/dtb.c. Can we initialise the backend > > correctly > > with the rest of the environment variables in pdbg_default_dtb() such > > that > > pdbg_get_backend() just returns the "correct" thing? > > Sure. I was also not happy about having to do separate checks > elsewhere in the code. > > Are you ok with the environment variable PDBG_BACKEND_DRIVER? I > thought of using PDBG_BACKEND earlier, but we are specifying the > backend device tree, so that seemed odd to also specify PDBG_BACKEND. > That's why PDBG_BACKEND_DRIVER, which is backend setting for selecting > a collection of drivers. Yeah, I think PDBG_BACKEND_DRIVER makes the most sense. Thanks. - Alistair > Let me know what you prefer. > > > - Alistair > > > > > + if (backend != PDBG_DEFAULT_BACKEND) > > > + p = find_compatible(backend, compat_list, len); > > > + } else { > > > + p = find_compatible(backend, compat_list, len); > > > + } > > > > > > - p = find_compatible(pdbg_get_backend(), compat_list, len); > > > > > > if (!p) > > > > > > p = find_compatible(PDBG_DEFAULT_BACKEND, compat_list, > > > > > > len); > > Amitay.
diff --git a/libpdbg/hwunit.c b/libpdbg/hwunit.c index 074ddef..3d6a05d 100644 --- a/libpdbg/hwunit.c +++ b/libpdbg/hwunit.c @@ -14,6 +14,7 @@ * limitations under the License. */ +#include <stdlib.h> #include <string.h> #include <assert.h> @@ -70,12 +71,44 @@ static const struct hw_unit_info *find_compatible(enum pdbg_backend backend, return NULL; } +static enum pdbg_backend get_backend_driver(void) +{ + const char *tmp; + enum pdbg_backend backend = PDBG_DEFAULT_BACKEND; + + tmp = getenv("PDBG_BACKEND_DRIVER"); + if (tmp) { + if (!strcmp(tmp, "fsi")) + backend = PDBG_BACKEND_FSI; + else if (!strcmp(tmp, "i2c")) + backend = PDBG_BACKEND_I2C; + else if (!strcmp(tmp, "kernel")) + backend = PDBG_BACKEND_KERNEL; + else if (!strcmp(tmp, "fake")) + backend = PDBG_BACKEND_FAKE; + else if (!strcmp(tmp, "host")) + backend = PDBG_BACKEND_HOST; + else if (!strcmp(tmp, "cronus")) + backend = PDBG_BACKEND_CRONUS; + } + + return backend; +} + const struct hw_unit_info *pdbg_hwunit_find_compatible(const char *compat_list, uint32_t len) { - const struct hw_unit_info *p; + const struct hw_unit_info *p = NULL; + enum pdbg_backend backend = pdbg_get_backend(); + + if (backend == PDBG_DEFAULT_BACKEND) { + backend = get_backend_driver(); + if (backend != PDBG_DEFAULT_BACKEND) + p = find_compatible(backend, compat_list, len); + } else { + p = find_compatible(backend, compat_list, len); + } - p = find_compatible(pdbg_get_backend(), compat_list, len); if (!p) p = find_compatible(PDBG_DEFAULT_BACKEND, compat_list, len);
When PDBG_BACKEND_DTB is specified, backend may not be set. This means only the drivers registered with PDBG_DEFAULT_BACKEND will be loaded. To be able to match backend specific drivers for targets in system tree, use the backend specified in PDBG_BACKEND_DRIVER environment variable. Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- libpdbg/hwunit.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)