[Skiboot] [PATCH] Add in new OPAL call to flush the L2 and L3 caches.
Rashmica Gupta
rashmica.g at gmail.com
Fri Nov 2 15:19:26 AEDT 2018
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.
More information about the Skiboot
mailing list