[Pdbg] [PATCH v2 1/2] libpdbg: Add default device-tree selection to libpdbg

[Alistair Popple]

On Tuesday, 2 July 2019 5:52:26 PM AEST Amitay Isaacs wrote:
> Hi Alistair,
> 
> I could not apply the patch to the current master.

Hmm, ok. It must conflict with what was just pushed. Will rebase and resubmit 
with fixes for most of your comments. Thanks for the review!
 
> > +int pdbg_set_backend(enum pdbg_backend backend, const char
> > *backend_option)
> > +{
> > +	pdbg_backend = backend;
> > +	pdbg_backend_option = backend_option;
> > +
> > +	return 0;
> > +}
> 
> This can be a void function if we are not verifying the arguments.

This becomes a library API function, so whilst we don't currently validate the 
arguments it's something we could add in future. I suppose we should make sure 
we actually check the return value though in the caller.

> > +
> > +const char *pdbg_get_backend_option(void)
> > +{
> > +	return pdbg_backend_option;
> > +}
> > +
> > +/* Determines what platform we are running on and returns a pointer
> > to
> > + * the fdt that is most likely to work on the system. */
> > +void *pdbg_default_dtb(void)
> > +{
> > +	char *dtb = getenv("LPDBG_DTB");
> 
> Any reason why we don't just use PDBG_DTB?  Or if you want to make it
> explicit that it's a library DTB, then LIBPDBG_DTB might be better.

PDBG_DTB it is.

> > +
> > +	if (!pdbg_backend && dtb)
> > +		return mmap_dtb(dtb);
> > +
> > +	if (!pdbg_backend)
> > +		pdbg_backend = default_backend();
> 
> What happens if pdbg_backend and dtb, both are set?  From the code it
> just ignores dtb.  May be we should log something, so user knows what
> happened.

I was debating what to do here. The idea was if the application specifies a DTB 
we should always use that. However I think you're right - if the user 
explicitly specifies a device-tree we should always use that. An application 
that really wants to control which one is used can always just pass it into 
pdbg_targets_init() itself.

- Alistair

> > +
> > +	switch(pdbg_backend) {
> > +	case PDBG_BACKEND_HOST:
> > +		return ppc_target();
> > +		break;
> > +
> > +	case PDBG_BACKEND_I2C:
> > +		/* I2C is only supported on POWER8 */
> > +		pdbg_log(PDBG_INFO, "Found a POWER8 AMI BMC based
> > system\n");
> > +		return &_binary_p8_i2c_dtb_o_start;
> > +		break;
> > +
> > +	case PDBG_BACKEND_KERNEL:
> > +		return bmc_target();
> > +		break;
> > +
> > +	case PDBG_BACKEND_FSI:
> > +		if (!strcmp(pdbg_backend_option, "p8"))
> > +			return &_binary_p8_fsi_dtb_o_start;
> > +		else if (!strcmp(pdbg_backend_option, "p9w"))
> > +			return &_binary_p9w_fsi_dtb_o_start;
> > +		else if (!strcmp(pdbg_backend_option, "p9r"))
> > +			return &_binary_p9r_fsi_dtb_o_start;
> > +		else if (!strcmp(pdbg_backend_option, "p9z"))
> > +			return &_binary_p9z_fsi_dtb_o_start;
> > +		else {
> > +			pdbg_log(PDBG_WARNING, "Invalid device type
> > specified, using a fake one\n");
> > +			return &_binary_fake_dtb_o_start;
> > +		}
> > +
> > +		break;
> > +
> > +	default:
> > +		pdbg_log(PDBG_WARNING, "Unable to determine a valid
> > default backend, using a fake one for testing purposes\n");
> > +		/* Fall through */
> > +
> > +	case PDBG_BACKEND_FAKE:
> > +		return &_binary_fake_dtb_o_start;
> > +		break;
> > +	}
> > +}
> > diff --git a/libpdbg/i2c.c b/libpdbg/i2c.c
> > index 227c8a0..1a5d089 100644
> > --- a/libpdbg/i2c.c
> > +++ b/libpdbg/i2c.c
> > @@ -131,7 +131,11 @@ int i2c_target_probe(struct pdbg_target *target)
> > 
> >  	const char *bus;
> >  	int addr;
> > 
> > -	bus = pdbg_target_property(&pib->target, "bus", NULL);
> > +	bus = pdbg_get_backend_option();
> > +
> > +	if (!bus)
> > +		bus = pdbg_target_property(&pib->target, "bus", NULL);
> > +l, pathsel_count))
> +                       return false;
> > 
> >  	if (!bus)
> >  	
> >  		bus = "/dev/i2c4";
> > 
> > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> > index 14a41ab..ae763e5 100644
> > --- a/libpdbg/libpdbg.h
> > +++ b/libpdbg/libpdbg.h
> > @@ -52,6 +52,9 @@ enum pdbg_target_status {PDBG_TARGET_UNKNOWN = 0,
> > PDBG_TARGET_ENABLED,
> > 
> >  			 PDBG_TARGET_DISABLED, PDBG_TARGET_MUSTEXIST,
> >  			 PDBG_TARGET_NONEXISTENT,
> > 
> > PDBG_TARGET_RELEASED};
> > 
> > +enum pdbg_backend { PDBG_DEFAULT_BACKEND = 0, PDBG_BACKEND_FSI,
> > PDBG_BACKEND_I2C,
> > +		    PDBG_BACKEND_KERNEL, PDBG_BACKEND_FAKE,
> > PDBG_BACKEND_HOST };
> > +
> > 
> >  #define pdbg_for_each_compatible(parent, target, compat)
> > 
> > \
> > 
> >          for (target =
> > 
> > NULL;                                             \
> > 
> >               (target = __pdbg_next_compatible_node(parent, target,
> > 
> > compat)) != NULL;)
> > @@ -103,6 +106,8 @@ enum pdbg_target_status pdbg_target_probe(struct
> > pdbg_target *target);
> > 
> >  void pdbg_target_release(struct pdbg_target *target);
> >  enum pdbg_target_status pdbg_target_status(struct pdbg_target
> > 
> > *target);
> > 
> >  void pdbg_target_status_set(struct pdbg_target *target, enum
> > 
> > pdbg_target_status status);
> > +int pdbg_set_backend(enum pdbg_backend backend, const char
> > *backend_option);
> > +void *pdbg_default_dtb(void);
> > 
> >  uint32_t pdbg_target_index(struct pdbg_target *target);
> >  char *pdbg_target_path(const struct pdbg_target *target);
> >  struct pdbg_target *pdbg_target_from_path(struct pdbg_target
> > 
> > *target, const char *path);
> > diff --git a/libpdbg/target.h b/libpdbg/target.h
> > index 233f085..193c0e0 100644
> > --- a/libpdbg/target.h
> > +++ b/libpdbg/target.h
> > @@ -61,4 +61,6 @@ bool pdbg_target_is_class(struct pdbg_target
> > *target, const char *class);
> > 
> >  extern struct list_head empty_list;
> >  extern struct list_head target_classes;
> > 
> > +const char *pdbg_get_backend_option(void);
> > +
> > 
> >  #endif
> 
> Amitay.