Re: some mips changes for loongson2
Simon Burge
> On Wed, Aug 10, 2011 at 10:28:11PM +1000, Simon Burge wrote:
> > I _think_ I'd like to see a "j ra" comment on this, but not entirely
> > sure, as we don't use the "RETURN" convention anywhere else. That said,
> > this hardly looks great either:
> >
> > STATIC_LEAF(ras_atomic_cas_noupdate)
> > RETURN /* j ra on most CPUs */
> > move v0, t0
> > END(ras_atomic_cas_noupdate)
>
> I renamed it to J_RA as suggested by David.
Cool, that's a better suggestion.
> > > @@ -223,6 +237,13 @@ END(ras_mutex_vector_exit)
> > >
> > > .p2align LOG2_MIPS_LOCK_RAS_SIZE /* Get out of the RAS block */
> > >
> > > + .set at
> > > +#ifdef MIPS3_LOONGSON2F
> > > +loongson_return:
> > > + j ra
> > > + nop
> > > +#endif
> > > +
> >
> > Is this chunk missing the corresponding ".set noat"?
>
> No, it's on purpose. AFAIK there's no issue using the at register from
> here till the end of the file.
Looking at the conventions we use, it looks like we usually use ".set
noat" at the start of a particular function that needs that and then
return to ".set at" after so I'd say that the rest of this file doesn't
align with what we usually do and that you'd normally expect ".set at"
to be in effect. Based on that, I won't ask you to fix the rest of the
file that you didn't touch :)
> > Since that's the end of an else, I believe the convention for the endif
> > comment would be /* !MIPS3_LOONGSON2 */.
>
> You're probably right (I never clearly seen what the convention is :),
> I've changed my sources.
Heh, thanks.
> > > @@ -1774,8 +1838,8 @@ LEAF(MIPSX(tlb_update))
> > > tlbwi # update slot found
> > > COP0_SYNC
> > > #ifdef MIPS3_LOONGSON2
> > > - li k0, MIPS_DIAG_ITLB_CLEAR
> > > - mtc0 v0, MIPS_COP_0_DIAG # invalidate ITLB
> > > + li t1, MIPS_DIAG_ITLB_CLEAR
> > > + mtc0 t1, MIPS_COP_0_DIAG # invalidate ITLB
> >
> > Without understanding the Loongson2 code, this appears to be changing
> > the behaviour of this code (and similar code chunks below this). In the
> > existing code k0 is loaded but v0 is moved to the MIPS_COP_0_DIAG reg.
> > In your patch t1 is used for both. I'm guessing that the previous code
> > was buggy and using the same register is now correct?
>
> I forgot to comment on this, sorry. Yes, the previous code is buggy:
> in several case we load the value to write to cop0 in k0 and transfer
> v0 to cop0 instead. In addition, this specific case is also clobbering
> the return value of tlb_update(). using t1 here is safe.
Thanks, that makes perfect sense.
Overall thanks for addressing all my comments. You've got my OK (if
that's worth anything anymore!).
Cheers,
Simon.
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de