| Message ID | 1574694943-7883-1-git-send-email-yingjie_bai@126.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
| Series | powerpc/mpc85xx: also write addr_h to spin table for 64bit boot entry | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (2ec2260ce7bce5eb6a8ced0bb78d75c1b3eca306) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Mon, 2019-11-25 at 23:15 +0800, yingjie_bai@126.com wrote: > From: Bai Yingjie <byj.tea@gmail.com> > > CPU like P4080 has 36bit physical address, its DDR physical > start address can be configured above 4G by LAW registers. > > For such systems in which their physical memory start address was > configured higher than 4G, we need also to write addr_h into the spin > table of the target secondary CPU, so that addr_h and addr_l together > represent a 64bit physical address. > Otherwise the secondary core can not get correct entry to start from. > > This should do no harm for normal case where addr_h is all 0. > > Signed-off-by: Bai Yingjie <byj.tea@gmail.com> > --- > arch/powerpc/platforms/85xx/smp.c | 8 ++++++++ > 1 file changed, 8 insertions(+) Acked-by: Scott Wood <oss@buserror.net> -Scott
yingjie_bai@126.com writes: > From: Bai Yingjie <byj.tea@gmail.com> > > CPU like P4080 has 36bit physical address, its DDR physical > start address can be configured above 4G by LAW registers. > > For such systems in which their physical memory start address was > configured higher than 4G, we need also to write addr_h into the spin > table of the target secondary CPU, so that addr_h and addr_l together > represent a 64bit physical address. > Otherwise the secondary core can not get correct entry to start from. > > This should do no harm for normal case where addr_h is all 0. > > Signed-off-by: Bai Yingjie <byj.tea@gmail.com> > --- > arch/powerpc/platforms/85xx/smp.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c > index 8c7ea2486bc0..f12cdd1e80ff 100644 > --- a/arch/powerpc/platforms/85xx/smp.c > +++ b/arch/powerpc/platforms/85xx/smp.c > @@ -252,6 +252,14 @@ static int smp_85xx_start_cpu(int cpu) > out_be64((u64 *)(&spin_table->addr_h), > __pa(ppc_function_entry(generic_secondary_smp_init))); > #else > + /* > + * We need also to write addr_h to spin table for systems > + * in which their physical memory start address was configured > + * to above 4G, otherwise the secondary core can not get > + * correct entry to start from. > + * This does no harm for normal case where addr_h is all 0. > + */ > + out_be32(&spin_table->addr_h, __pa(__early_start) >> 32); > out_be32(&spin_table->addr_l, __pa(__early_start)); This breaks the corenet32_smp_defconfig build: /linux/arch/powerpc/platforms/85xx/smp.c: In function 'smp_85xx_start_cpu': /linux/arch/powerpc/platforms/85xx/smp.c:262:52: error: right shift count >= width of type [-Werror=shift-count-overflow] 262 | out_be32(&spin_table->addr_h, __pa(__early_start) >> 32); | ^~ cc1: all warnings being treated as errors cheers
Hi Michael, Thanks for pointing out the issue. My mistake... This patch should indeed make sense only when CONFIG_PHYS_64BIT=y I could not find corenet32_smp_defconfig, but I guess in your config, CONFIG_PHYS_64BIT=n ? I will update the patch later today On Sun, Dec 22, 2019 at 5:38 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > yingjie_bai@126.com writes: > > From: Bai Yingjie <byj.tea@gmail.com> > > > > CPU like P4080 has 36bit physical address, its DDR physical > > start address can be configured above 4G by LAW registers. > > > > For such systems in which their physical memory start address was > > configured higher than 4G, we need also to write addr_h into the spin > > table of the target secondary CPU, so that addr_h and addr_l together > > represent a 64bit physical address. > > Otherwise the secondary core can not get correct entry to start from. > > > > This should do no harm for normal case where addr_h is all 0. > > > > Signed-off-by: Bai Yingjie <byj.tea@gmail.com> > > --- > > arch/powerpc/platforms/85xx/smp.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c > > index 8c7ea2486bc0..f12cdd1e80ff 100644 > > --- a/arch/powerpc/platforms/85xx/smp.c > > +++ b/arch/powerpc/platforms/85xx/smp.c > > @@ -252,6 +252,14 @@ static int smp_85xx_start_cpu(int cpu) > > out_be64((u64 *)(&spin_table->addr_h), > > __pa(ppc_function_entry(generic_secondary_smp_init))); > > #else > > + /* > > + * We need also to write addr_h to spin table for systems > > + * in which their physical memory start address was configured > > + * to above 4G, otherwise the secondary core can not get > > + * correct entry to start from. > > + * This does no harm for normal case where addr_h is all 0. > > + */ > > + out_be32(&spin_table->addr_h, __pa(__early_start) >> 32); > > out_be32(&spin_table->addr_l, __pa(__early_start)); > > This breaks the corenet32_smp_defconfig build: > > /linux/arch/powerpc/platforms/85xx/smp.c: In function 'smp_85xx_start_cpu': > /linux/arch/powerpc/platforms/85xx/smp.c:262:52: error: right shift count >= width of type [-Werror=shift-count-overflow] > 262 | out_be32(&spin_table->addr_h, __pa(__early_start) >> 32); > | ^~ > cc1: all warnings being treated as errors > > cheers
On Tue, 2019-12-24 at 09:35 +0800, Yingjie Bai wrote: > Hi Michael, > Thanks for pointing out the issue. My mistake... > This patch should indeed make sense only when > CONFIG_PHYS_64BIT=y > > I could not find corenet32_smp_defconfig, but I guess in your config, > CONFIG_PHYS_64BIT=n ? > I will update the patch later today corenet32_smp_defconfig is a makefile rule that pulls in multiple config fragments. It has CONFIG_PHYS_64BIT=y, but __pa() returns an unsigned long regardless (which obviously needs to be fixed if DDR starting beyond 4G is to be supported). What 32-bit config are you using where this actually builds? -Scott
Hi Scott, __pa() returns 64bit in my setup. in arch/powerpc/include/asm/page.h #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + VIRT_PHYS_OFFSET)) #define __pa(x) ((unsigned long)(x) - VIRT_PHYS_OFFSET) #else #ifdef CONFIG_PPC64 ... /* See Description below for VIRT_PHYS_OFFSET */ #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) #ifdef CONFIG_RELOCATABLE #define VIRT_PHYS_OFFSET virt_phys_offset #else #define VIRT_PHYS_OFFSET (KERNELBASE - PHYSICAL_START) #endif #endif and VIRT_PHYS_OFFSET is a variable, virt_phys_offset, which is long long in arch/powerpc/mm/init_32.c #ifdef CONFIG_RELOCATABLE /* Used in __va()/__pa() */ long long virt_phys_offset; EXPORT_SYMBOL(virt_phys_offset); #endif my config has CONFIG_RELOCATABLE=y CONFIG_BOOKE=y CONFIG_FSL_BOOKE=y CONFIG_PPC_FSL_BOOK3E=y CONFIG_PPC_BOOK3E_MMU=y CONFIG_FSL_SOC_BOOKE=y CONFIG_FSL_CORENET_RCPM=y CONFIG_CORENET_GENERIC=y CONFIG_VDSO32=y CONFIG_PPC32=y CONFIG_32BIT=y CONFIG_ARCH_HAS_ILOG2_U32=y CONFIG_INIT_ENV_ARG_LIMIT=32 # CONFIG_PPC64 is not set CONFIG_PTE_64BIT=y CONFIG_PHYS_64BIT=y CONFIG_ARCH_PHYS_ADDR_T_64BIT=y CONFIG_ARCH_DMA_ADDR_T_64BIT=y # CONFIG_HAVE_64BIT_ALIGNED_ACCESS is not set CONFIG_PHYS_ADDR_T_64BIT=y CONFIG_PCI_BUS_ADDR_T_64BIT=y CONFIG_XZ_DEC_IA64=y CONFIG_GENERIC_ATOMIC64=y # CONFIG_ATOMIC64_SELFTEST is not set CONFIG_PRINT_STACK_DEPTH=64 Here is my .config taken from a customized kernel, version 4.9 # # Automatically generated file; DO NOT EDIT. # Linux/powerpc 4.9.79 Kernel Configuration # # CONFIG_PPC64 is not set # # Processor support # # CONFIG_PPC_BOOK3S_32 is not set CONFIG_PPC_85xx=y # CONFIG_PPC_8xx is not set # CONFIG_40x is not set # CONFIG_44x is not set # CONFIG_E200 is not set CONFIG_E500=y CONFIG_PPC_E500MC=y CONFIG_PPC_FPU=y CONFIG_FSL_EMB_PERFMON=y CONFIG_FSL_EMB_PERF_EVENT=y CONFIG_FSL_EMB_PERF_EVENT_E500=y CONFIG_BOOKE=y CONFIG_FSL_BOOKE=y CONFIG_PPC_FSL_BOOK3E=y CONFIG_PTE_64BIT=y CONFIG_PHYS_64BIT=y CONFIG_PPC_MMU_NOHASH=y CONFIG_PPC_BOOK3E_MMU=y # CONFIG_PPC_MM_SLICES is not set CONFIG_SMP=y CONFIG_NR_CPUS=4 CONFIG_PPC_DOORBELL=y CONFIG_VDSO32=y CONFIG_CPU_BIG_ENDIAN=y CONFIG_PPC32=y CONFIG_32BIT=y CONFIG_ARCH_PHYS_ADDR_T_64BIT=y CONFIG_ARCH_DMA_ADDR_T_64BIT=y CONFIG_MMU=y # CONFIG_HAVE_SETUP_PER_CPU_AREA is not set # CONFIG_NEED_PER_CPU_EMBED_FIRST_CHUNK is not set CONFIG_NR_IRQS=512 CONFIG_STACKTRACE_SUPPORT=y CONFIG_TRACE_IRQFLAGS_SUPPORT=y CONFIG_LOCKDEP_SUPPORT=y CONFIG_RWSEM_XCHGADD_ALGORITHM=y CONFIG_GENERIC_LOCKBREAK=y CONFIG_ARCH_HAS_ILOG2_U32=y CONFIG_GENERIC_HWEIGHT=y CONFIG_ARCH_HAS_DMA_SET_COHERENT_MASK=y CONFIG_PPC=y # CONFIG_GENERIC_CSUM is not set CONFIG_EARLY_PRINTK=y CONFIG_PANIC_TIMEOUT=180 CONFIG_GENERIC_NVRAM=y CONFIG_SCHED_OMIT_FRAME_POINTER=y CONFIG_ARCH_MAY_HAVE_PC_FDC=y CONFIG_PPC_UDBG_16550=y CONFIG_GENERIC_TBSYNC=y CONFIG_AUDIT_ARCH=y CONFIG_GENERIC_BUG=y # CONFIG_EPAPR_BOOT is not set CONFIG_DEFAULT_UIMAGE=y CONFIG_ARCH_HIBERNATION_POSSIBLE=y # CONFIG_PPC_DCR_NATIVE is not set # CONFIG_PPC_DCR_MMIO is not set CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y CONFIG_ARCH_SUPPORTS_UPROBES=y CONFIG_PPC_ADV_DEBUG_REGS=y CONFIG_PPC_ADV_DEBUG_IACS=2 CONFIG_PPC_ADV_DEBUG_DACS=2 CONFIG_PPC_ADV_DEBUG_DVCS=0 CONFIG_PPC_ADV_DEBUG_DAC_RANGE=y CONFIG_PGTABLE_LEVELS=2 CONFIG_DEFCONFIG_LIST="/lib/modules/$UNAME_RELEASE/.config" CONFIG_IRQ_WORK=y # # Platform support # # CONFIG_PPC_CELL is not set # CONFIG_PPC_CELL_NATIVE is not set # CONFIG_PQ2ADS is not set CONFIG_FSL_SOC_BOOKE=y # CONFIG_BSC9131_RDB is not set # CONFIG_C293_PCIE is not set # CONFIG_BSC9132_QDS is not set # CONFIG_MPC8540_ADS is not set # CONFIG_MPC8560_ADS is not set # CONFIG_MPC85xx_CDS is not set # CONFIG_MPC85xx_MDS is not set # CONFIG_MPC8536_DS is not set # CONFIG_MPC85xx_DS is not set # CONFIG_MPC85xx_RDB is not set # CONFIG_P1010_RDB is not set # CONFIG_P1022_DS is not set # CONFIG_P1022_RDK is not set # CONFIG_P1023_RDB is not set # CONFIG_TWR_P102x is not set # CONFIG_SOCRATES is not set # CONFIG_KSI8560 is not set # CONFIG_XES_MPC85xx is not set # CONFIG_STX_GP3 is not set # CONFIG_TQM8540 is not set # CONFIG_TQM8541 is not set # CONFIG_TQM8548 is not set # CONFIG_TQM8555 is not set # CONFIG_TQM8560 is not set # CONFIG_SBC8548 is not set # CONFIG_PPA8548 is not set # CONFIG_GE_IMP3A is not set # CONFIG_SGY_CTS1000 is not set # CONFIG_MVME2500 is not set # CONFIG_PPC_QEMU_E500 is not set CONFIG_CORENET_GENERIC=y # CONFIG_KVM_GUEST is not set CONFIG_EPAPR_PARAVIRT=y CONFIG_PPC_SMP_MUXED_IPI=y # CONFIG_IPIC is not set CONFIG_MPIC=y # CONFIG_MPIC_TIMER is not set CONFIG_PPC_EPAPR_HV_PIC=y # CONFIG_MPIC_WEIRD is not set CONFIG_MPIC_MSGR=y # CONFIG_PPC_I8259 is not set # CONFIG_PPC_RTAS is not set # CONFIG_MMIO_NVRAM is not set # CONFIG_MPIC_U3_HT_IRQS is not set # CONFIG_PPC_MPC106 is not set # CONFIG_PPC_970_NAP is not set # CONFIG_PPC_P7_NAP is not set On Wed, Dec 25, 2019 at 9:25 AM Scott Wood <oss@buserror.net> wrote: > > On Tue, 2019-12-24 at 09:35 +0800, Yingjie Bai wrote: > > Hi Michael, > > Thanks for pointing out the issue. My mistake... > > This patch should indeed make sense only when > > CONFIG_PHYS_64BIT=y > > > > I could not find corenet32_smp_defconfig, but I guess in your config, > > CONFIG_PHYS_64BIT=n ? > > I will update the patch later today > > corenet32_smp_defconfig is a makefile rule that pulls in multiple config > fragments. It has CONFIG_PHYS_64BIT=y, but __pa() returns an unsigned long > regardless (which obviously needs to be fixed if DDR starting beyond 4G is to > be supported). > > What 32-bit config are you using where this actually builds? > > -Scott > >
On Wed, 2019-12-25 at 11:24 +0800, Yingjie Bai wrote: > Hi Scott, > > __pa() returns 64bit in my setup. > > in arch/powerpc/include/asm/page.h > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + > VIRT_PHYS_OFFSET)) > #define __pa(x) ((unsigned long)(x) - VIRT_PHYS_OFFSET) > #else > #ifdef CONFIG_PPC64 > ... > > > > /* See Description below for VIRT_PHYS_OFFSET */ > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > #ifdef CONFIG_RELOCATABLE > #define VIRT_PHYS_OFFSET virt_phys_offset > #else > #define VIRT_PHYS_OFFSET (KERNELBASE - PHYSICAL_START) > #endif > #endif OK, so it's the lack of CONFIG_RELOCATABLE causing the build to fail. Ideally we'd make __pa() consistently return phys_addr_t, even if the upper bits are known to always be zero in a particular config. -Scott
Thanks Scott, I will test to see if returning phys_addr_t in __pa() works for my setup. And another thin I will test is to compile without CONFIG_RELOCATABLE before resubmitting the patch. On Wed, Dec 25, 2019 at 2:53 PM Scott Wood <oss@buserror.net> wrote: > > On Wed, 2019-12-25 at 11:24 +0800, Yingjie Bai wrote: > > Hi Scott, > > > > __pa() returns 64bit in my setup. > > > > in arch/powerpc/include/asm/page.h > > > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + > > VIRT_PHYS_OFFSET)) > > #define __pa(x) ((unsigned long)(x) - VIRT_PHYS_OFFSET) > > #else > > #ifdef CONFIG_PPC64 > > ... > > > > > > > > /* See Description below for VIRT_PHYS_OFFSET */ > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > > #ifdef CONFIG_RELOCATABLE > > #define VIRT_PHYS_OFFSET virt_phys_offset > > #else > > #define VIRT_PHYS_OFFSET (KERNELBASE - PHYSICAL_START) > > #endif > > #endif > > OK, so it's the lack of CONFIG_RELOCATABLE causing the build to fail. Ideally > we'd make __pa() consistently return phys_addr_t, even if the upper bits are > known to always be zero in a particular config. > > -Scott > >
Hi Scott, Thanks for your time to review this patch Based on your suggestion, I have verified below new patches that pass compilation with and without CONFIG_RELOCATABLE https://lore.kernel.org/patchwork/patch/1173548 https://lore.kernel.org/patchwork/patch/1173547 On Wed, Dec 25, 2019 at 3:19 PM Yingjie Bai <byj.tea@gmail.com> wrote: > > Thanks Scott, I will test to see if returning phys_addr_t in __pa() > works for my setup. > > And another thin I will test is to compile without CONFIG_RELOCATABLE > before resubmitting the patch. > > On Wed, Dec 25, 2019 at 2:53 PM Scott Wood <oss@buserror.net> wrote: > > > > On Wed, 2019-12-25 at 11:24 +0800, Yingjie Bai wrote: > > > Hi Scott, > > > > > > __pa() returns 64bit in my setup. > > > > > > in arch/powerpc/include/asm/page.h > > > > > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > > > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + > > > VIRT_PHYS_OFFSET)) > > > #define __pa(x) ((unsigned long)(x) - VIRT_PHYS_OFFSET) > > > #else > > > #ifdef CONFIG_PPC64 > > > ... > > > > > > > > > > > > /* See Description below for VIRT_PHYS_OFFSET */ > > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > > > #ifdef CONFIG_RELOCATABLE > > > #define VIRT_PHYS_OFFSET virt_phys_offset > > > #else > > > #define VIRT_PHYS_OFFSET (KERNELBASE - PHYSICAL_START) > > > #endif > > > #endif > > > > OK, so it's the lack of CONFIG_RELOCATABLE causing the build to fail. Ideally > > we'd make __pa() consistently return phys_addr_t, even if the upper bits are > > known to always be zero in a particular config. > > > > -Scott > > > >
Hi Scott, Sorry for the late reply, I have checked the compilation error from kbuild system, and found when CONFIG_PHYS_64BIT is not set, phys_addr_t is 32bit, there is still "right shift count >= width of type" error. So I update the patches accordingly. https://lore.kernel.org/patchwork/patch/1175560/ https://lore.kernel.org/patchwork/patch/1175559/ On Mon, Dec 30, 2019 at 3:41 PM Yingjie Bai <byj.tea@gmail.com> wrote: > > Hi Scott, > > Thanks for your time to review this patch > > Based on your suggestion, I have verified below new patches that pass > compilation with and without CONFIG_RELOCATABLE > > https://lore.kernel.org/patchwork/patch/1173548 > https://lore.kernel.org/patchwork/patch/1173547 > > On Wed, Dec 25, 2019 at 3:19 PM Yingjie Bai <byj.tea@gmail.com> wrote: > > > > Thanks Scott, I will test to see if returning phys_addr_t in __pa() > > works for my setup. > > > > And another thin I will test is to compile without CONFIG_RELOCATABLE > > before resubmitting the patch. > > > > On Wed, Dec 25, 2019 at 2:53 PM Scott Wood <oss@buserror.net> wrote: > > > > > > On Wed, 2019-12-25 at 11:24 +0800, Yingjie Bai wrote: > > > > Hi Scott, > > > > > > > > __pa() returns 64bit in my setup. > > > > > > > > in arch/powerpc/include/asm/page.h > > > > > > > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > > > > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + > > > > VIRT_PHYS_OFFSET)) > > > > #define __pa(x) ((unsigned long)(x) - VIRT_PHYS_OFFSET) > > > > #else > > > > #ifdef CONFIG_PPC64 > > > > ... > > > > > > > > > > > > > > > > /* See Description below for VIRT_PHYS_OFFSET */ > > > > #if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE) > > > > #ifdef CONFIG_RELOCATABLE > > > > #define VIRT_PHYS_OFFSET virt_phys_offset > > > > #else > > > > #define VIRT_PHYS_OFFSET (KERNELBASE - PHYSICAL_START) > > > > #endif > > > > #endif > > > > > > OK, so it's the lack of CONFIG_RELOCATABLE causing the build to fail. Ideally > > > we'd make __pa() consistently return phys_addr_t, even if the upper bits are > > > known to always be zero in a particular config. > > > > > > -Scott > > > > > >
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c index 8c7ea2486bc0..f12cdd1e80ff 100644 --- a/arch/powerpc/platforms/85xx/smp.c +++ b/arch/powerpc/platforms/85xx/smp.c @@ -252,6 +252,14 @@ static int smp_85xx_start_cpu(int cpu) out_be64((u64 *)(&spin_table->addr_h), __pa(ppc_function_entry(generic_secondary_smp_init))); #else + /* + * We need also to write addr_h to spin table for systems + * in which their physical memory start address was configured + * to above 4G, otherwise the secondary core can not get + * correct entry to start from. + * This does no harm for normal case where addr_h is all 0. + */ + out_be32(&spin_table->addr_h, __pa(__early_start) >> 32); out_be32(&spin_table->addr_l, __pa(__early_start)); #endif flush_spin_table(spin_table);