diff mbox series

[v4,05/19] libpdbg: Use PDBG_BACKEND_DRIVER to specify drivers to load

Message ID 20200421041655.82856-6-amitay@ozlabs.org
State Superseded
Headers show
Series Add sbefifo backend | expand

Checks

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

Commit Message

Amitay Isaacs April 21, 2020, 4:16 a.m. UTC
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(-)

Comments

Alistair Popple April 30, 2020, 12:07 a.m. UTC | #1
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);
Amitay Isaacs April 30, 2020, 1:22 a.m. UTC | #2
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.
Alistair Popple April 30, 2020, 2:17 a.m. UTC | #3
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 mbox series

Patch

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