Re: [PATCH v5 2/5] target/ppc: Add Power11 DD2.0 processor

[Aditya Gupta <adityag@linux.ibm.com>]

Hi Nick,


On 23/07/24 10:00, Nicholas Piggin wrote:
>> <...snip...>
>>
>> +    { /* POWER11, ISA3.10 */
>> +        .name = "power11",
>> +        .pvr = CPU_POWERPC_LOGICAL_3_10_PLUS,
> Might call that _P11 rather than _PLUS, but I can fold that in my tree.
Sure, makes sense, I can make these changes, and send a v2 soon.
>> <...snip...>
>>
>> +
>> +POWERPC_FAMILY(POWER11)(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>> +
>> +    dc->fw_name = "PowerPC,POWER11";
>> +    dc->desc = "POWER11";
>> +    pcc->pvr_match = ppc_pvr_match_power11;
>> +    pcc->pcr_mask = POWERPC_POWER10_PCC_PCR_MASK;
>> +    pcc->pcr_supported = POWERPC_POWER10_PCC_PCR_SUPPORTED;
>> +    pcc->init_proc = init_proc_POWER10;
>> +    pcc->check_pow = check_pow_nocheck;
>> +    pcc->check_attn = check_attn_hid0_power9;
>> +    pcc->insns_flags = POWERPC_FAMILY_POWER9_INSNS_FLAGS; /* same as P9 */
>> +    pcc->insns_flags2 = POWERPC_FAMILY_POWER10_INSNS_FLAGS2;
>> +    pcc->msr_mask = POWERPC_POWER10_PCC_MSR_MASK;
>> +    pcc->lpcr_mask = POWERPC_POWER10_PCC_LPCR_MASK;
> BTW., I still think all these new macros should be named after the exact
> CPU, e.g., all these should be called POWER11 and the differences or
> sameness should be handled in cpu_init.h.

Got it, can create macros for the Power11 things also.


Regarding this:

> +    pcc->check_attn = check_attn_hid0_power9;
> +    pcc->insns_flags = POWERPC_FAMILY_POWER9_INSNS_FLAGS; /* same as P9 */

Should I keep them same, or have *_POWER11_* counterparts ?
>
> I might tweak that and the names a bit locally (e.g., why is one type of
> define called POWERPC_FAMILY_x and another called POWERPC_x_PCC), but
> that's not a big deal and mostly an exercise in bike shed painting. The
> functionality of the patch looks okay.
I am okay if you want to do it, or i can do it in a separate follow up 
patch.
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks for the tag Nick !


- Aditya Gupta

>
>> +
>> +    pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
>> +    pcc->mmu_model = POWERPC_MMU_3_00;
>> +#if !defined(CONFIG_USER_ONLY)
>> +    /* segment page size remain the same */
>> +    pcc->hash64_opts = &ppc_hash64_opts_POWER7;
>> +    pcc->radix_page_info = &POWER10_radix_page_info;
>> +    pcc->lrg_decr_bits = 56;
>> +#endif
>> +    pcc->excp_model = POWERPC_EXCP_POWER10;
>> +    pcc->bus_model = PPC_FLAGS_INPUT_POWER9;
>> +    pcc->bfd_mach = bfd_mach_ppc64;
>> +    pcc->flags = POWERPC_POWER10_PCC_FLAGS;
>> +    pcc->l1_dcache_size = 0x8000;
>> +    pcc->l1_icache_size = 0x8000;
>> +}
>> +
>>   #if !defined(CONFIG_USER_ONLY)
>>   void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>>   {