[Skiboot] [PATCH] Add in new OPAL call to flush the L2 and L3 caches.

[Rashmica Gupta]

On Fri, 2018-11-02 at 14:14 +1100, Alexey Kardashevskiy wrote:
> > > > 
> 
> On 02/11/2018 13:48, Rashmica Gupta wrote:
> > On Fri, 2018-11-02 at 13:17 +1100, Alexey Kardashevskiy wrote:
> > > 
> > > On 29/10/2018 17:19, Rashmica Gupta wrote:

...

> > > > 
> > > > +static int flush_l2_caches(uint32_t chip_id, uint32_t core_id)
> > > > +{
> > > > +	int rc, timeout = 0;
> > > > +	unsigned long start_time;
> > > > +	uint64_t val = L2_PRD_PURGE_CMD_REG_BUSY;
> > > > +	uint64_t addr = XSCOM_ADDR_P9_EX(core_id,
> > > > L2_PRD_PURGE_CMD_REG);
> > > > +
> > > > +	rc = xscom_write_mask(chip_id, addr,
> > > > L2_PRD_PURGE_CMD_TRIGGER,
> > > > +			      L2_PRD_PURGE_CMD_TRIGGER);
> > > 
> > > The advise from the hw folks was:
> > > 
> > >  putspy pu.ex EXP.L2.L2MISC.L2CERRS.PRD_PURGE_CMD_TYPE
> > > L2CAC_FLUSH
> > > -all
> > > 
> > >  putspy pu.ex EXP.L2.L2MISC.L2CERRS.PRD_PURGE_CMD_TRIGGER ON -all
> > > 
> > > I can see trigger action but not L2CAC_FLUSH.
> > > 
> > 
> > I lumped them together (as L2CAC_FLUSH=>0b0000). I'll split it up
> > and
> > make it clearer though.
> 
> 
> Ah, my bad, I was reading it as 0x0b0000 and was looking for 'b'
> :)Anyway, useful to see them defined somewhere. Also, can it be a
> single
> write to the register (looks like it can and then there is no need to
> separation) or should it be 2 writes as the hw folks strangely
> suggested?

I assumed that it would be fine to write the type of flush and trigger
it at the same time... But their suggestion did have them seperate, so 
maybe I should do it like that just to be safe?

> 
> When you post another version, please include those 'putspy' commands
> to
> the commit log for the reference.
> 
> 

Ok :)

> > 
> > > > +	if (rc) {
> > > > +		prlog(PR_ERR, "FLUSH L2 on core 0x%x: XSCOM
> > > > write_mask
> > > > failed %i\n", core_id, rc);
> > > 
> > > Any reason not to "return rc" right here?
> > 
> > Not that I can think of. 
> > 
> > > 
> > > > +	}
> > > > +	start_time = tb_to_msecs(mftb());
> > > > +	while ((val & L2_PRD_PURGE_CMD_REG_BUSY) && !(timeout =
> > > > time_expired(start_time))) {
> > > 
> > > 
> > > Can we please not have '=' in a condition?
> > > 
> > 
> > That will disapear with Al's suggestion.
> > 
> > > 
> > > > +		rc = xscom_read(chip_id, addr, &val);
> > > > +		if (rc) {
> > > > +			prlog(PR_ERR, "FLUSH L2 on core 0x%x:
> > > > XSCOM
> > > > read failed %i\n", core_id, rc);
> > > > +			break;
> > > > +		}
> > > 
> > > 
> > > if (tb_to_msecs(mftb()) - start_time > TIMEOUT_MS) {
> > > 	prlog(PR_ERR, "FLUSH L3 on core 0x%x timed out %i\n", core_id,
> > > rc);
> > > 	return OPAL_BUSY;
> > > }
> > > 
> > > ...
> > > 
> > > 
> > > > +	}
> > > > +	if (timeout) {
> > > > +		prlog(PR_ERR, "FLUSH L3 on core 0x%x timed out
> > > > %i\n",
> > > > core_id, rc);
> > > > +		return OPAL_BUSY;
> > > > +	}
> > > 
> > > ... and ditch this check. 
> > 
> > Why? You don't care if we timed out trying to clear the caches?
> 
> I meant if you check-and-return for timeout inside the loop without
> any
> extra @timeout variable, you won't need this one.
> 

Ah, gotcha.