On 11/11/20 10:17 AM, cupertinomira...@gmail.com wrote: > +enum arc_cpu_family { > + ARC_OPCODE_NONE = 0, > + ARC_OPCODE_DEFAULT = 1 << 0, > + ARC_OPCODE_ARC600 = 1 << 1, > + ARC_OPCODE_ARC700 = 1 << 2, > + ARC_OPCODE_ARCv2EM = 1 << 3, > + ARC_OPCODE_ARCv2HS = 1 << 4 > +};
Indentation is off. > +/*-*-indent-tabs-mode:nil;tab-width:4;indent-line-function:'insert-tab'-*-*/ > +/* vim: set ts=4 sw=4 et: */ This ought to be covered by the top-level .editorconfig. > +static void arc_cpu_set_pc(CPUState *cs, vaddr value) > +{ > + ARCCPU *cpu = ARC_CPU(cs); > + > + CPU_PCL(&cpu->env) = value & 0xfffffffc; > + cpu->env.pc = value; > +} Indentation is off all through cpu.c. > +#ifndef CPU_ARC_H > +#define CPU_ARC_H > + > +#include "cpu-qom.h" > +#include "exec/cpu-defs.h" > +#include "fpu/softfloat.h" You probably only need softfloat-types.h in cpu.h. > +#define MMU_IDX 0 What's this? > +#define PHYS_BASE_RAM 0x00000000 > +#define VIRT_BASE_RAM 0x00000000 This seems like board stuff, not cpu stuff. > +enum gdb_regs { > + GDB_REG_0 = 0, > + GDB_REG_1, > + GDB_REG_2, > + GDB_REG_3, > + GDB_REG_4, > + GDB_REG_5, > + GDB_REG_6, > + GDB_REG_7, > + GDB_REG_8, > + GDB_REG_9, > + GDB_REG_10, > + GDB_REG_11, > + GDB_REG_12, > + GDB_REG_13, > + GDB_REG_14, > + GDB_REG_15, > + GDB_REG_16, > + GDB_REG_17, > + GDB_REG_18, > + GDB_REG_19, > + GDB_REG_20, > + GDB_REG_21, > + GDB_REG_22, > + GDB_REG_23, > + GDB_REG_24, > + GDB_REG_25, > + GDB_REG_26, /* GP */ > + GDB_REG_27, /* FP */ > + GDB_REG_28, /* SP */ > + GDB_REG_29, /* ILINK */ > + GDB_REG_30, /* R30 */ > + GDB_REG_31, /* BLINK */ > + GDB_REG_58, /* little_endian? ACCL : ACCH */ > + GDB_REG_59, /* little_endian? ACCH : ACCL */ > + GDB_REG_60, /* LP */ > + GDB_REG_63, /* Immediate */ > + GDB_REG_LAST > +}; Why is this in cpu.h and not gdbstub.c? > +#define CPU_GP(env) ((env)->r[26]) > +#define CPU_FP(env) ((env)->r[27]) > +#define CPU_SP(env) ((env)->r[28]) > +#define CPU_ILINK(env) ((env)->r[29]) > +#define CPU_ILINK1(env) ((env)->r[29]) > +#define CPU_ILINK2(env) ((env)->r[30]) > +#define CPU_BLINK(env) ((env)->r[31]) > +#define CPU_LP(env) ((env)->r[60]) > +#define CPU_IMM(env) ((env)->r[62]) > +#define CPU_PCL(env) ((env)->r[63]) Does it make more sense to define these in terms of numbers than lvalue macros? > +typedef struct status_register { > + uint32_t Hf; /* halt */ > + uint32_t Ef; /* irq priority treshold. */ > + uint32_t AEf; > + uint32_t DEf; > + uint32_t Uf; > + uint32_t Vf; /* overflow */ > + uint32_t Cf; /* carry */ > + uint32_t Nf; /* negative */ > + uint32_t Zf; /* zero */ > + uint32_t Lf; > + uint32_t DZf; > + uint32_t SCf; > + uint32_t ESf; > + uint32_t RBf; > + uint32_t ADf; > + uint32_t USf; > + uint32_t IEf; > + > + /* Reserved bits */ > + > + /* Next instruction is a delayslot instruction */ > + uint32_t is_delay_slot_instruction; > +} status_t; "status_t" is much too general a name, and is not unlikely to conflict with something from somewhere else. Why are you exploding all of the bits of status anyway? I would think that only VCNZ should be handled specially, and only for the current context. You should be using CamelCase as per CODING_STYLE, and probably using a prefix of Arc or ARC for *everything*. > + > +/* ARC processor timer module. */ > +typedef struct { > + /* > + * TODO: This volatile is needed to pass RTC tests. We need to > + * verify why. > + */ > + volatile uint32_t T_Cntrl; > + volatile uint32_t T_Limit; > + volatile uint64_t last_clk; That is a serious bug, probably incorrect usage of locks. > +typedef struct CPUARCState { > + uint32_t r[64]; > + > + status_t stat, stat_l1, stat_er; > + > + struct { > + uint32_t S2; > + uint32_t S1; > + uint32_t CS; > + } macmod; > + > + uint32_t intvec; > + > + uint32_t eret; > + uint32_t erbta; > + uint32_t ecr; > + uint32_t efa; > + uint32_t bta; > + uint32_t bta_l1; > + uint32_t bta_l2; > + > + uint32_t pc; /* program counter */ Why is this here? Surely it's redundant with r[63]. > + struct { > + uint32_t LD; /* load pending bit */ > + uint32_t SH; /* self halt */ > + uint32_t BH; /* breakpoint halt */ > + uint32_t UB; /* user mode break enabled */ > + uint32_t ZZ; /* sleep mode */ > + uint32_t RA; /* reset applied */ > + uint32_t IS; /* single instruction step */ > + uint32_t FH; /* force halt */ > + uint32_t SS; /* single step */ > + } debug; Why are these bits expanded to words? > + /* used for propagatinng "hostpc/return address" to sub-functions */ > + uintptr_t host_pc; This is a bad idea, IMO. > + /* Fields after this point are preserved across CPU reset. */ > + uint64_t features; > + uint32_t family; Usually such fields belong in ArchCPU (ArcCPU) and not CPUArchState. > + /* ARC Configuration Settings. */ > + struct { > + uint32_t addr_size; > + bool aps_feature; > + bool byte_order; > + bool bitscan_option; > + uint32_t br_bc_entries; Please sort the fields by size/alignment. This ordering is wasteful. > +/* are we in user mode? */ > +static inline bool is_user_mode(const CPUARCState *env) > +{ > + return env->stat.Uf != false; > +} Well, ignoring for the moment that stat should not be expanded to words, this is a silly way to return a bool. Better as just return env->stat.Uf. > +static inline int arc_feature(const CPUARCState *env, int feature) Return bool. > +static inline void arc_set_feature(CPUARCState *env, int feature) extra space. > +static inline int cpu_mmu_index(const CPUARCState *env, bool ifetch) > +{ > + return env->stat.Uf != 0 ? 1 : 0; > +} That's a silly way to write Uf != 0. > + *pc = env->pc; > + *cs_base = 0; > +#ifdef CONFIG_USER_ONLY > + *pflags = TB_FLAGS_FP_ENABLE; > +#else > + *pflags = cpu_mmu_index(env, 0); > +#endif Why does !CONFIG_USER_ONLY never set TB_FLAGS_FP_ENABLE? > +static inline int cpu_interrupts_enabled(const CPUARCState *env) > +{ > + return env->stat.IEf; > +} This is not used. Copy and paste from some other target? > +/* these are the helper functions used both by translation and gdbstub */ > +target_ulong helper_lr(CPUARCState *env, uint32_t aux); > +void helper_sr(CPUARCState *env, uint32_t val, uint32_t aux); Don't declare these twice, just include "exec/helper-proto.h" in gdbstub.c. r~ _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc