[Pdbg] [PATCH v2 3/3] Add fircheck support

[Balbir Singh]

On Fri, 18 May 2018 11:52:21 +1000
Alistair Popple <alistair at popple.id.au> wrote:

> Thanks for looking into this Balbir, I have a couple of comments below.
> 
> > +static uint64_t global_mc_fir_addr;
> > +
> > +extern int for_each_target(char *class, int (*cb)(struct pdbg_target *, uint32_t, uint64_t *, uint64_t *),
> > +				uint64_t *arg1, uint64_t *arg2);  
> 
> I'm trying to make this function go away as the library now has some iterators
> which are nice to use than callbacks with random arguments and should cover most
> cases. Eg. pdbg_for_each_target(...).
>

OK, we should mark them as deprecated if the function is really going away.

> > +static int getfir(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *ignored)
> > +{
> > +	int rc;
> > +	uint64_t value;
> > +	struct pdbg_target *dn;
> > +	uint32_t i;
> > +
> > +	rc = pib_read(target, *addr, &value);
> > +	if (value == 0)
> > +		printf("%s%d: No checkstop detected\n", "p", index);
> > +
> > +	dt_for_each_compatible(dt_root, dn, "ibm,power9-chiplet") {
> > +		i = dt_prop_get_u32(dn, "index");
> > +		if (!i)
> > +			continue; /* we should abort really */
> > +		if (value & PPC_BIT(i)) {
> > +			printf("%s%d: FIR bit set for chiplet %s\n", "p", index, dn->dn_name);  
> 
> Ideally I'd rather see any user output in the application (src/*) rather than
> the library. Which I guess means this function should just return the actual FIR
> bit mask and the leave the analysis/printing to the application.

OK, I think the whole function can move to src/p9fir.c, which is where I had
them, otherwise too much data needs to passed back and forth

> > + */
> > +int handle_fircheck(int optind, int argc, char *argv[])
> > +{
> > +	struct pdbg_target *global_fir_dn;
> > +	int rc;
> > +
> > +	global_fir_dn = dt_find_by_name(dt_root, "global_fir");
> > +	if (!global_fir_dn) {
> > +		rc = -1;
> > +		PR_ERROR("Failed to find global multicast scom fir address"
> > +			 "Please check the device tree bindings\n");
> > +		goto out;
> > +	}
> > +
> > +	if (!global_mc_fir_addr)
> > +		global_mc_fir_addr = dt_get_address(global_fir_dn, 0, NULL);
> > +	if (!global_mc_fir_addr) {
> > +		rc = -1;
> > +		PR_ERROR("Failed to find global multicast scom fir address"
> > +			 "Please check the device tree bindings\n");
> > +		goto out;
> > +	}  
> 
> It would be nice if we could move the calculation of FIR address into the
> library though. So maybe add this to getfir() above and have getfir() return the
> result which can loop over chiplet numbers here? I guess we could also return a
> list of struct pdbg_target pointers which have a xstop rather than the bitmask,
> but that's probably overkill for now at least.

OK, I'll refactor the code

> 
> > +	rc = for_each_target("pib", getfir, &global_mc_fir_addr, NULL);  
> 
> I'd rather we use the iterators here as they're safer and more readable IMHO.
> Eg:
> 
> pdbg_for_each_class_target("pib", target) {
> 	/* Find chiplet target by index and print out name if xstopped */
> }

OK


Balbir Singh.