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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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

- Ayan

> Cheers,