Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Zhao Ke <ke.zhao@shingroup.cn>, npiggin@gmail.com,
        christophe.leroy@csgroup.eu, fbarrat@linux.ibm.com,
        ajd@linux.ibm.com, arnd@arndb.de, gregkh@linuxfoundation.org
Cc:     linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, Zhao Ke <ke.zhao@shingroup.cn>
Subject: Re: [PATCH] powerpc: Add PVN support for HeXin C2000 processor
In-Reply-To: <20231117075215.647-1-ke.zhao@shingroup.cn>
References: <20231117075215.647-1-ke.zhao@shingroup.cn>
Date:   Wed, 22 Nov 2023 12:46:51 +1100
Message-ID: <87sf4yk19w.fsf@mail.lhotse>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Zhao Ke <ke.zhao@shingroup.cn> writes:
> HeXin Tech Co. has applied for a new PVN from the OpenPower Community
> for its new processor C2000. The OpenPower has assigned a new PVN
> and this newly assigned PVN is 0x0066, add pvr register related
> support for this PVN.
>
> Signed-off-by: Zhao Ke <ke.zhao@shingroup.cn>
> Link: https://discuss.openpower.foundation/t/how-to-get-a-new-pvr-for-processors-follow-power-isa/477/10
 
Hi Zhao Ke,

Thanks for the patch. Just a few questions.

Are you able to provide any further detail on the processor?

Your cputable entry claims that it's identical to the original Power8
core, can you comment at all on how true that is in practice?

Unfortunately the kernel has some hard-coded knowledge of various
non-architected features, which are not controlled via the CPU table,
and are instead controlled by firmware. So you'll need to make sure you
set those correctly, see init_fw_feat_flags() for details.

One other comment below ...

> diff --git a/arch/powerpc/kernel/cpu_specs_book3s_64.h b/arch/powerpc/kernel/cpu_specs_book3s_64.h
> index c370c1b804a9..4f604934da7c 100644
> --- a/arch/powerpc/kernel/cpu_specs_book3s_64.h
> +++ b/arch/powerpc/kernel/cpu_specs_book3s_64.h
> @@ -238,6 +238,21 @@ static struct cpu_spec cpu_specs[] __initdata = {
>  		.machine_check_early	= __machine_check_early_realmode_p8,
>  		.platform		= "power8",
>  	},
> +	{	/* 2.07-compliant processor, HeXin C2000 processor */
> +		.pvr_mask		= 0xffffffff,
> +		.pvr_value		= 0x00660000,
> +		.cpu_name		= "POWER8 (architected)",
 
Using "(architected)" here is not right. That's reserved for the
0x0f00000x range of PVRs.

You should use "POWER8 (raw)", or you could actually use the marketing
name there if you want to, eg. "HeXin C2000" or whatever.

cheers