| Message ID | 3e0f5249da1e943da65e0cea0f907a1c7bc3271a.1435723168.git.serge.vakulenko@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2015-06-30 21:12, Serge Vakulenko wrote: > Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> > --- > target-mips/cpu.h | 2 ++ > target-mips/translate_init.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/target-mips/cpu.h b/target-mips/cpu.h > index ab830ee..9f5890c 100644 > --- a/target-mips/cpu.h > +++ b/target-mips/cpu.h > @@ -394,6 +394,7 @@ struct CPUMIPSState { > #define CP0C0_M 31 > #define CP0C0_K23 28 > #define CP0C0_KU 25 > +#define CP0C0_SB 21 Bits in the range 16:24 are implementation specific, so I do wonder if we want to have this bit there. At least we should mark it as implementation specific. > #define CP0C0_MDU 20 > #define CP0C0_MM 17 > #define CP0C0_BM 16 > @@ -479,6 +480,7 @@ struct CPUMIPSState { > #define CP0C5_NFExists 0 > int32_t CP0_Config6; > int32_t CP0_Config7; > +#define CP0C7_WII 31 Same as above, Config6 and Config7 are implementation dependent. > /* XXX: Maybe make LLAddr per-TC? */ > uint64_t lladdr; > target_ulong llval; > diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c > index ddfaff8..430a547 100644 > --- a/target-mips/translate_init.c > +++ b/target-mips/translate_init.c > @@ -232,6 +232,52 @@ static const mips_def_t mips_defs[] = > .mmu_type = MMU_TYPE_FMT, > }, > { > + /* Configuration for Microchip PIC32MX microcontroller. */ > + .name = "M4K", > + .CP0_PRid = 0x00018765, > + .CP0_Config0 = MIPS_CONFIG0 | (2 << CP0C0_K23) | (2 << CP0C0_KU) | > + (1 << CP0C0_SB) | (1 << CP0C0_BM) | > + (1 << CP0C0_AR) | (MMU_TYPE_FMT << CP0C0_MT), > + .CP0_Config1 = (1U << CP0C1_M) | (1 << CP0C1_CA) | (1 << CP0C1_EP), > + .CP0_Config2 = MIPS_CONFIG2, > + .CP0_Config3 = (1 << CP0C3_VEIC) | (1 << CP0C3_VInt), > + .CP0_LLAddr_rw_bitmask = 0, > + .CP0_LLAddr_shift = 4, > + .SYNCI_Step = 32, > + .CCRes = 2, > + .CP0_Status_rw_bitmask = 0x1258FF17, > + .SEGBITS = 32, > + .PABITS = 32, > + .insn_flags = CPU_MIPS32R2 | ASE_MIPS16, > + .mmu_type = MMU_TYPE_FMT, > + }, > + { > + /* Configuration for Microchip PIC32MZ microcontroller. */ > + .name = "microAptivP", > + .CP0_PRid = 0x00019e28, > + .CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) | > + (MMU_TYPE_R4000 << CP0C0_MT), > + .CP0_Config1 = MIPS_CONFIG1 | (15 << CP0C1_MMU) | (1 << CP0C1_PC), > + .CP0_Config2 = MIPS_CONFIG2, > + .CP0_Config3 = (1 << CP0C3_M) | (1 << CP0C3_IPLW) | (1 << CP0C3_MCU) | > + (2 << CP0C3_ISA) | (1 << CP0C3_ULRI) | (1 << CP0C3_RXI) | > + (1 << CP0C3_DSP2P) | (1 << CP0C3_DSPP) | (1 << CP0C3_VEIC) | > + (1 << CP0C3_VInt), DSP and DSPr2 are enabled here... > + .CP0_Config4 = (1 << CP0C4_M), > + .CP0_Config5 = (1 << CP0C5_NFExists), > + .CP0_Config6 = 0, > + .CP0_Config7 = (1 << CP0C7_WII), > + .CP0_LLAddr_rw_bitmask = 0, > + .CP0_LLAddr_shift = 4, > + .SYNCI_Step = 32, > + .CCRes = 2, > + .CP0_Status_rw_bitmask = 0x1278FF17, > + .SEGBITS = 32, > + .PABITS = 32, > + .insn_flags = CPU_MIPS32R2, so I guess you want to enable ASE_DSP and ASE_DSPR2 here. > + .mmu_type = MMU_TYPE_R4000, > + }, > + { > .name = "24Kc", > .CP0_PRid = 0x00019300, > .CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) | Otherwise it looks ok, though I haven't look at the PIC32 manual to check the values.
On Wed, 1 Jul 2015, Aurelien Jarno wrote: > > diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c > > index ddfaff8..430a547 100644 > > --- a/target-mips/translate_init.c > > +++ b/target-mips/translate_init.c > > @@ -232,6 +232,52 @@ static const mips_def_t mips_defs[] = > > .mmu_type = MMU_TYPE_FMT, > > }, > > { > > + /* Configuration for Microchip PIC32MX microcontroller. */ > > + .name = "M4K", > > + .CP0_PRid = 0x00018765, Hmm, does it make sense to set the Revision field here? We keep it at 0 for other templates, so why not 0x00018700? Also I suggest to move the template earlier on so that entries remain sorted by PRId, at least within the same vendor. So this would go between "4KEmR1" and "4KEc" (the M4K is an MTI RTL, quite an old one actually). > > + { > > + /* Configuration for Microchip PIC32MZ microcontroller. */ > > + .name = "microAptivP", > > + .CP0_PRid = 0x00019e28, Same question here, why not 0x00019e00? Also why not "microAptivUP" as documentation calls it (vs "microAptivUC")? And again, it looks to me like the entry better followed "M14Kc". > Otherwise it looks ok, though I haven't look at the PIC32 manual to > check the values. I haven't checked if the bit patterns for configuration registers are sane either. These RTLs are configurable, so (within some limits) real hardware will have different values anyway. Maciej
On Wed, Jul 1, 2015 at 6:37 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2015-06-30 21:12, Serge Vakulenko wrote: >> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> >> --- >> target-mips/cpu.h | 2 ++ >> target-mips/translate_init.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/target-mips/cpu.h b/target-mips/cpu.h >> index ab830ee..9f5890c 100644 >> --- a/target-mips/cpu.h >> +++ b/target-mips/cpu.h >> @@ -394,6 +394,7 @@ struct CPUMIPSState { >> #define CP0C0_M 31 >> #define CP0C0_K23 28 >> #define CP0C0_KU 25 >> +#define CP0C0_SB 21 > > Bits in the range 16:24 are implementation specific, so I do wonder if > we want to have this bit there. At least we should mark it as > implementation specific. I tried to make the configuration as close as possible to a real PIC32 microcontroller - that's why I added Config0.SB and Config7.WII bits. These bits are described in appropriate Microchip docs. As they are not relevant for the simulation purposes, I'll better remove them for simplicity. >> #define CP0C0_MDU 20 >> #define CP0C0_MM 17 >> #define CP0C0_BM 16 >> @@ -479,6 +480,7 @@ struct CPUMIPSState { >> #define CP0C5_NFExists 0 >> int32_t CP0_Config6; >> int32_t CP0_Config7; >> +#define CP0C7_WII 31 > > Same as above, Config6 and Config7 are implementation dependent. > >> /* XXX: Maybe make LLAddr per-TC? */ >> uint64_t lladdr; >> target_ulong llval; >> diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c >> index ddfaff8..430a547 100644 >> --- a/target-mips/translate_init.c >> +++ b/target-mips/translate_init.c >> @@ -232,6 +232,52 @@ static const mips_def_t mips_defs[] = >> .mmu_type = MMU_TYPE_FMT, >> }, >> { >> + /* Configuration for Microchip PIC32MX microcontroller. */ >> + .name = "M4K", >> + .CP0_PRid = 0x00018765, >> + .CP0_Config0 = MIPS_CONFIG0 | (2 << CP0C0_K23) | (2 << CP0C0_KU) | >> + (1 << CP0C0_SB) | (1 << CP0C0_BM) | >> + (1 << CP0C0_AR) | (MMU_TYPE_FMT << CP0C0_MT), >> + .CP0_Config1 = (1U << CP0C1_M) | (1 << CP0C1_CA) | (1 << CP0C1_EP), >> + .CP0_Config2 = MIPS_CONFIG2, >> + .CP0_Config3 = (1 << CP0C3_VEIC) | (1 << CP0C3_VInt), >> + .CP0_LLAddr_rw_bitmask = 0, >> + .CP0_LLAddr_shift = 4, >> + .SYNCI_Step = 32, >> + .CCRes = 2, >> + .CP0_Status_rw_bitmask = 0x1258FF17, >> + .SEGBITS = 32, >> + .PABITS = 32, >> + .insn_flags = CPU_MIPS32R2 | ASE_MIPS16, >> + .mmu_type = MMU_TYPE_FMT, >> + }, >> + { >> + /* Configuration for Microchip PIC32MZ microcontroller. */ >> + .name = "microAptivP", >> + .CP0_PRid = 0x00019e28, >> + .CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) | >> + (MMU_TYPE_R4000 << CP0C0_MT), >> + .CP0_Config1 = MIPS_CONFIG1 | (15 << CP0C1_MMU) | (1 << CP0C1_PC), >> + .CP0_Config2 = MIPS_CONFIG2, >> + .CP0_Config3 = (1 << CP0C3_M) | (1 << CP0C3_IPLW) | (1 << CP0C3_MCU) | >> + (2 << CP0C3_ISA) | (1 << CP0C3_ULRI) | (1 << CP0C3_RXI) | >> + (1 << CP0C3_DSP2P) | (1 << CP0C3_DSPP) | (1 << CP0C3_VEIC) | >> + (1 << CP0C3_VInt), > > DSP and DSPr2 are enabled here... > >> + .CP0_Config4 = (1 << CP0C4_M), >> + .CP0_Config5 = (1 << CP0C5_NFExists), >> + .CP0_Config6 = 0, >> + .CP0_Config7 = (1 << CP0C7_WII), >> + .CP0_LLAddr_rw_bitmask = 0, >> + .CP0_LLAddr_shift = 4, >> + .SYNCI_Step = 32, >> + .CCRes = 2, >> + .CP0_Status_rw_bitmask = 0x1278FF17, >> + .SEGBITS = 32, >> + .PABITS = 32, >> + .insn_flags = CPU_MIPS32R2, > > so I guess you want to enable ASE_DSP and ASE_DSPR2 here. Makes sense. Thank you for noticing this. >> + .mmu_type = MMU_TYPE_R4000, >> + }, >> + { >> .name = "24Kc", >> .CP0_PRid = 0x00019300, >> .CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) | > > Otherwise it looks ok, though I haven't look at the PIC32 manual to > check the values. > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net
On Fri, Jul 3, 2015 at 3:04 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote: > On Wed, 1 Jul 2015, Aurelien Jarno wrote: > >> > diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c >> > index ddfaff8..430a547 100644 >> > --- a/target-mips/translate_init.c >> > +++ b/target-mips/translate_init.c >> > @@ -232,6 +232,52 @@ static const mips_def_t mips_defs[] = >> > .mmu_type = MMU_TYPE_FMT, >> > }, >> > { >> > + /* Configuration for Microchip PIC32MX microcontroller. */ >> > + .name = "M4K", >> > + .CP0_PRid = 0x00018765, > > Hmm, does it make sense to set the Revision field here? We keep it at 0 > for other templates, so why not 0x00018700? OK, I will zero out the Revision field, to align with the rest of configurations. > Also I suggest to move the template earlier on so that entries remain > sorted by PRId, at least within the same vendor. So this would go between > "4KEmR1" and "4KEc" (the M4K is an MTI RTL, quite an old one actually). Not a problem, I'll reorder it. >> > + { >> > + /* Configuration for Microchip PIC32MZ microcontroller. */ >> > + .name = "microAptivP", >> > + .CP0_PRid = 0x00019e28, > > Same question here, why not 0x00019e00? Also why not "microAptivUP" as > documentation calls it (vs "microAptivUC")? Well... I don't see any reason not to change it to "microAptivUP", for consistency with MIPS documentation. > And again, it looks to me like the entry better followed "M14Kc". > >> Otherwise it looks ok, though I haven't look at the PIC32 manual to >> check the values. > > I haven't checked if the bit patterns for configuration registers are > sane either. These RTLs are configurable, so (within some limits) real > hardware will have different values anyway. > > Maciej
On 2015-07-05 20:48, Serge Vakulenko wrote: > On Wed, Jul 1, 2015 at 6:37 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2015-06-30 21:12, Serge Vakulenko wrote: > >> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> > >> --- > >> target-mips/cpu.h | 2 ++ > >> target-mips/translate_init.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 48 insertions(+) > >> > >> diff --git a/target-mips/cpu.h b/target-mips/cpu.h > >> index ab830ee..9f5890c 100644 > >> --- a/target-mips/cpu.h > >> +++ b/target-mips/cpu.h > >> @@ -394,6 +394,7 @@ struct CPUMIPSState { > >> #define CP0C0_M 31 > >> #define CP0C0_K23 28 > >> #define CP0C0_KU 25 > >> +#define CP0C0_SB 21 > > > > Bits in the range 16:24 are implementation specific, so I do wonder if > > we want to have this bit there. At least we should mark it as > > implementation specific. > > I tried to make the configuration as close as possible to a real PIC32 > microcontroller - that's why I added Config0.SB and Config7.WII bits. > These bits are described in appropriate Microchip docs. As they are > not relevant for the simulation purposes, I'll better remove them for > simplicity. It's fine if they are needed, but I suggest in that case to chose a name showing it's PIC32 specific, something like CP0C0_PIC32_SB.
diff --git a/target-mips/cpu.h b/target-mips/cpu.h index ab830ee..9f5890c 100644 --- a/target-mips/cpu.h +++ b/target-mips/cpu.h @@ -394,6 +394,7 @@ struct CPUMIPSState { #define CP0C0_M 31 #define CP0C0_K23 28 #define CP0C0_KU 25 +#define CP0C0_SB 21 #define CP0C0_MDU 20 #define CP0C0_MM 17 #define CP0C0_BM 16 @@ -479,6 +480,7 @@ struct CPUMIPSState { #define CP0C5_NFExists 0 int32_t CP0_Config6; int32_t CP0_Config7; +#define CP0C7_WII 31 /* XXX: Maybe make LLAddr per-TC? */ uint64_t lladdr; target_ulong llval; diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c index ddfaff8..430a547 100644 --- a/target-mips/translate_init.c +++ b/target-mips/translate_init.c @@ -232,6 +232,52 @@ static const mips_def_t mips_defs[] = .mmu_type = MMU_TYPE_FMT, }, { + /* Configuration for Microchip PIC32MX microcontroller. */ + .name = "M4K", + .CP0_PRid = 0x00018765, + .CP0_Config0 = MIPS_CONFIG0 | (2 << CP0C0_K23) | (2 << CP0C0_KU) | + (1 << CP0C0_SB) | (1 << CP0C0_BM) | + (1 << CP0C0_AR) | (MMU_TYPE_FMT << CP0C0_MT), + .CP0_Config1 = (1U << CP0C1_M) | (1 << CP0C1_CA) | (1 << CP0C1_EP), + .CP0_Config2 = MIPS_CONFIG2, + .CP0_Config3 = (1 << CP0C3_VEIC) | (1 << CP0C3_VInt), + .CP0_LLAddr_rw_bitmask = 0, + .CP0_LLAddr_shift = 4, + .SYNCI_Step = 32, + .CCRes = 2, + .CP0_Status_rw_bitmask = 0x1258FF17, + .SEGBITS = 32, + .PABITS = 32, + .insn_flags = CPU_MIPS32R2 | ASE_MIPS16, + .mmu_type = MMU_TYPE_FMT, + }, + { + /* Configuration for Microchip PIC32MZ microcontroller. */ + .name = "microAptivP", + .CP0_PRid = 0x00019e28, + .CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) | + (MMU_TYPE_R4000 << CP0C0_MT), + .CP0_Config1 = MIPS_CONFIG1 | (15 << CP0C1_MMU) | (1 << CP0C1_PC), + .CP0_Config2 = MIPS_CONFIG2, + .CP0_Config3 = (1 << CP0C3_M) | (1 << CP0C3_IPLW) | (1 << CP0C3_MCU) | + (2 << CP0C3_ISA) | (1 << CP0C3_ULRI) | (1 << CP0C3_RXI) | + (1 << CP0C3_DSP2P) | (1 << CP0C3_DSPP) | (1 << CP0C3_VEIC) | + (1 << CP0C3_VInt), + .CP0_Config4 = (1 << CP0C4_M), + .CP0_Config5 = (1 << CP0C5_NFExists), + .CP0_Config6 = 0, + .CP0_Config7 = (1 << CP0C7_WII), + .CP0_LLAddr_rw_bitmask = 0, + .CP0_LLAddr_shift = 4, + .SYNCI_Step = 32, + .CCRes = 2, + .CP0_Status_rw_bitmask = 0x1278FF17, + .SEGBITS = 32, + .PABITS = 32, + .insn_flags = CPU_MIPS32R2, + .mmu_type = MMU_TYPE_R4000, + }, + { .name = "24Kc", .CP0_PRid = 0x00019300, .CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) |
Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> --- target-mips/cpu.h | 2 ++ target-mips/translate_init.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)