Re: [PATCH v1] xen/arm: arm32: Add support to identify the Cortex-R52 processor

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 22/06/2023 17:32, Julien Grall wrote:
> Hi,
Hi Julien,
> On 22/06/2023 16:41, Ayan Kumar Halder wrote:
> > On 22/06/2023 10:22, Julien Grall wrote:
> > > Hi Ayan,
> > Hi Julien,
> > > On 22/06/2023 09:59, Ayan Kumar Halder wrote:
> > > > On 20/06/2023 21:43, Julien Grall wrote:
> > > > > Hi Ayan,
> > > > Hi Julien,
> > > > > On 20/06/2023 19:28, Ayan Kumar Halder wrote:
> > > > > > On 20/06/2023 17:41, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > Hi Julien,
> > > > > > > On 20/06/2023 16:17, Ayan Kumar Halder wrote:
> > > > > > > > Add a special configuration (CONFIG_AARCH32_V8R) to setup the 
> > > > > > > > Cortex-R52
> > > > > > > > specifics.
> > > > > > > >
> > > > > > > > Cortex-R52 is an Arm-V8R AArch32 processor.
> > > > > > > >
> > > > > > > > Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR,
> > > > > > > > bits[31:24] = 0x41 , Arm Ltd
> > > > > > > > bits[23:20] = Implementation defined
> > > > > > > > bits[19:16] = 0xf , Arch features are individually identified
> > > > > > > > bits[15:4] = Implementation defined
> > > > > > > > bits[3:0] = Implementation defined
> > > > > > > >
> > > > > > > > Thus, the processor id is 0x410f0000 and the processor id mask is
> > > > > > > > 0xff0f0000
> > > > > > > >
> > > > > > > > Also, there is no special initialization required for R52.
> > > > > > > Are you saying that Xen upstream + this patch will boot on 
> > > > > > > Cortex-R52?
> > > > > > This patch will help for earlyboot of Xen. With this patch, 
> > > > > > cpu_init() will work on Cortex-R52.
> > > > > > There will be changes required for the MPU configuration, but 
> > > > > > that will be sent after Penny's patch serie "[PATCH v2 00/41] 
> > > > > > xen/arm: Add Armv8-R64 MPU support to Xen - Part#1" is upstreamed.
> > > > > > My aim is to extract the non-dependent changes and send them for 
> > > > > > review.
> > > > > I can review the patch. But I am not willing to merge it as it 
> > > > > gives the false impression that Xen would boot on Cortex-R52.
> > > > > In fact, I think this patch should only be merged once we have all 
> > > > > the MPU merged.
> > > > > IMHO, patches are independent are rework (e.g. code split...) that 
> > > > > would help the MPU.
> > > > Yes, that's exactly what I intend to do. I will send out the 
> > > > patches (similar to this) which will not be impacted by the MPU 
> > > > changes.
> > > > So, that both as an author/reviewer, it helps to restrict MPU serie 
> > > > to only MPU specific changes. >
> > > > Can you suggest some change to the commit message, so that we can 
> > > > commit it (without giving any false impression that Xen boots on 
> > > > Cortex-R52) >
> > > > May be adding this line to the commit message helps ? >
> > > > "However, Xen does not fully boot on Cortex-R52 as it requires MPU 
> > > > support which is under review. >
> > > > Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to 
> > > > reflect it."
> > > While writing an answer for this patch, I was actually wondering 
> > > whether the CPU allowlist still make sense for the 32-bit ARMv8-R?
> > > From Armv7-A, we needed it because some CPUs need specific quirk 
> > > when booting. But it would be best if we can get rid of it for 
> > > 32-bit ARMv8-R.
> > > Looking at your patch, your merely providing stubs. Do you have any 
> > > plan to fill them up?
> > Actually, I would use cr52_init in a later patch to write to CNTFRQ. 
> > See below :-
> > +#define XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ  800000U
> > +
> >   cr52_init:
> > +        /*
> > +         * Set counter frequency  800 KHZ
> > +         *
> > +         * Set counter frequency, CNTFRQ
> > +         */
> > +        ldr     r0,=XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ
> > +        mrc     15,0,r1,c14,c0,0  /* Read CNTFRQ */
> > +        cmp     r1,#0
> > +        /* Set only if the frequency read is 0 */
> > +        mcreq   15,0,r0,c14,c0,0  /* Write CNTFRQ */
> > +
> >           mov pc, lr
> Why can't you use the device-tree (see clock-frequency) as all the 
> other buggy platform does?
That's a good suggestion :). Also, I can do this in the platform 
specific .init().
Then, can I add the empty stubs for R52 (similar to 
https://elixir.bootlin.com/linux/latest/source/arch/arm/mm/proc-v7m.S#L165 
Cortex-M7, cpu_v7m_proc_init(), cpu_v7m_switch_mm() are stubs).
Or do you propose something like this :-

--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -322,7 +322,7 @@ cpu_init:
         PRINT("- Setting up control registers -\r\n")

         mov   r5, lr                       /* r5 := return address */
-
+#ifndef CONFIG_ARM_NO_PROC_INIT
         /* Get processor specific proc info into r1 */
         bl    __lookup_processor_type
         teq   r1, #0
@@ -337,7 +337,7 @@ cpu_init:
         ldr   r1, [r1, #PROCINFO_cpu_init]  /* r1 := vaddr(init func) */
         adr   lr, cpu_init_done             /* Save return address */
         add   pc, r1, r10                   /* Call paddr(init func) */
-
+#endif
 cpu_init_done:


And we introduce this new bool Kconfig option "CONFIG_ARM_NO_PROC_INIT" 
which will be defined for processors that do not need any special init 
(eg R52).
- Ayan

> Cheers,