Re: [PATCH v5 1/2] PRU Simulator port

[Dimitar Dimitrov <dimitar@dinux.eu>]

On Wed, 28 Aug 2019, 15:11:34 EEST Andrew Burgess wrote:
> * Dimitar Dimitrov <dimitar@dinux.eu> [2019-08-26 07:08:16 +0300]:
> > I'd like to contribute a sim port for the TI PRU I/O processor. This
> > is the fifth version of the patch series.
> > 
> > The GCC and binutils ports have already been merged. I've sent
> > patches for newlib.
> > 
> > Changes since patch series v4:
> >   - Rebased to latest top-of-tree.
> >   - Added XCHG instruction support.
> > 
> > v1: https://sourceware.org/ml/gdb-patches/2016-12/msg00143.html
> > v2: https://sourceware.org/ml/gdb-patches/2017-02/msg00397.html
> > v3: https://sourceware.org/ml/gdb-patches/2017-02/msg00516.html
> > v4: https://sourceware.org/ml/gdb-patches/2018-06/msg00484.html
> 
> Thanks for sticking with the patch over the years!  I built and tested
> this, and all the tests passed.  I have a few (hopefully) minor nits,
> but this is all looking pretty good.
Thank you for the review. I'll resubmit v6 to address all your comments.

> 
...
> 
> > 	* configure: Regenerate.
> 
> This file appears to be missing from the patch.
Unfortunaly the autogenerated configure file is rather large. I'll send it as a separate patch.

>
...
> > +   the performance of the simulator.  Simulation speed is deemed more
> > +   important.  */
> > +
> > +/* Please keep the same order as the GP registers.  */
> 
> It isn't clear to me where the order in "... same order as the GP
> registers" comes from.  Is this order defined in the ISA?  Defined in
> some other file in GDB or the simulator?   Could you expand on this
> comment maybe?
This enum order is purely easthetical and has no functional impact.
I hope the following C comment would sched some light. If it causes too
much confusion, I can altogether remove the comment. The relevant ISA
documentation can be found on page 31, section 5.2.3.2 in
https://elinux.org/images/d/da/Am335xPruReferenceGuide.pdf .


+/* For clarity, please keep the same relative order in this enum as in the 
+  corresponding group of GP registers. 
+
+  In PRU ISA, Multiplier-Accumulator-Unit's registers are like "shadows" of 
+  the GP registers.   MAC registers are implicitly addressed when executing 
+  the XIN/XOUT instructions to access them.  Transfer to/from a MAC register 
+  can happen only from/to its corresponding GP peer register.  */ 
+
+enum pru_macreg_id { 
+   /* MAC register       CPU GP register     Description.  */ 
+   PRU_MACREG_MODE,      /* r25 */           /* Mode (MUL/MAC).  */ 
+   PRU_MACREG_PROD_L,    /* r26 */           /* Lower 32 bits of product.  */ 
+   PRU_MACREG_PROD_H,    /* r27 */           /* Higher 32 bits of product.  */ 
+   PRU_MACREG_OP_0,      /* r28 */           /* First operand.  */ 
+   PRU_MACREG_OP_1,      /* r29 */           /* Second operand.  */ 
+   PRU_MACREG_ACC_L,     /* N/A */           /* Accumulator (not exposed)  */ 
+   PRU_MACREG_ACC_H,     /* N/A */           /* Higher 32 bits of MAC 
+                                                accumulator.  */ 
+   PRU_MAC_NREGS 
+};


> 
> > +enum pru_macreg_id {
> > +    PRU_MACREG_MODE,	  /* r25 */
> > +    PRU_MACREG_PROD_L,	  /* r26 */
> > +    PRU_MACREG_PROD_H,	  /* r27 */
> > +    PRU_MACREG_OP_0,	  /* r28 */
> > +    PRU_MACREG_OP_1,	  /* r29 */
> > +    PRU_MACREG_ACC_L,	  /* N/A */
> > +    PRU_MACREG_ACC_H,	  /* N/A */
> > +    PRU_MAC_NREGS
> > +};
> > +

Regards,
Dimitar