Messages in this thread |  | | Subject | Re: [PATCH v2 01/18] MIPS: intel: Add initial support for Intel MIPS SoCs | From | Hua Ma <> | Date | Mon, 6 Aug 2018 17:12:41 +0800 |
| |
On 8/4/2018 1:49 AM, Paul Burton wrote: > Hi Songjun / Hua, > > On Fri, Aug 03, 2018 at 11:02:20AM +0800, Songjun Wu wrote: >> From: Hua Ma <hua.ma@linux.intel.com> >> >> Add initial support for Intel MIPS interAptiv SoCs made by Intel. >> This series will add support for the grx500 family. >> >> The series allows booting a minimal system using a initramfs. > Thanks for submitting this - I have some comments below. Thanks for the review.
>> diff --git a/arch/mips/Kbuild.platforms b/arch/mips/Kbuild.platforms >> index ac7ad54f984f..bcd647060f3e 100644 >> --- a/arch/mips/Kbuild.platforms >> +++ b/arch/mips/Kbuild.platforms >> @@ -12,6 +12,7 @@ platforms += cobalt >> platforms += dec >> platforms += emma >> platforms += generic >> +platforms += intel-mips >> platforms += jazz >> platforms += jz4740 >> platforms += lantiq > Oh EVA, why must you ruin nice things... Ideally I'd be suggesting that > we use the generic platform but it doesn't yet have a nice way to deal > with non-standard EVA setups. yes, we only support EVA.
> It would be good if we could make sure that's the only reason for your > custom platform though, so that once generic does support EVA we can > migrate your system over. Most notably, it would be good to make use of > the UHI-specified boot protocol if possible (ie. $r4==-2, $r5==&dtb). > > It looks like your prom_init_cmdline() supports multiple boot protocols > - could you clarify which is actually used? this patch only support build-in dts, we will do a clean up.
>> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig >> index 08c10c518f83..2d34f17f3e24 100644 >> --- a/arch/mips/Kconfig >> +++ b/arch/mips/Kconfig >> @@ -409,6 +409,34 @@ config LANTIQ >> select ARCH_HAS_RESET_CONTROLLER >> select RESET_CONTROLLER >> >> +config INTEL_MIPS >> + bool "Intel MIPS interAptiv SoC based platforms" >> + select BOOT_RAW >> + select CEVT_R4K >> + select COMMON_CLK >> + select CPU_MIPS32_3_5_EVA >> + select CPU_MIPS32_3_5_FEATURES >> + select CPU_MIPSR2_IRQ_EI >> + select CPU_MIPSR2_IRQ_VI >> + select CSRC_R4K >> + select DMA_NONCOHERENT >> + select GENERIC_ISA_DMA >> + select IRQ_MIPS_CPU >> + select MFD_CORE >> + select MFD_SYSCON >> + select MIPS_CPU_SCACHE >> + select MIPS_GIC >> + select SYS_HAS_CPU_MIPS32_R1 > For a system based on interAptiv you should never need to build a > MIPS32r1 kernel, so you should remove the above select. will remove.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h b/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h >> new file mode 100644 >> index 000000000000..ac5f3b943d2a >> --- /dev/null >> +++ b/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h >> @@ -0,0 +1,61 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * This file was derived from: include/asm-mips/cpu-features.h >> + * Copyright (C) 2003, 2004 Ralf Baechle >> + * Copyright (C) 2004 Maciej W. Rozycki >> + * Copyright (C) 2018 Intel Corporation. >> + */ >> + >> +#ifndef __ASM_MACH_INTEL_MIPS_CPU_FEATURE_OVERRIDES_H >> +#define __ASM_MACH_INTEL_MIPS_CPU_FEATURE_OVERRIDES_H >> + >> +#define cpu_has_tlb 1 >> +#define cpu_has_4kex 1 >> +#define cpu_has_3k_cache 0 >> +#define cpu_has_4k_cache 1 >> +#define cpu_has_tx39_cache 0 >> +#define cpu_has_sb1_cache 0 >> +#define cpu_has_fpu 0 >> +#define cpu_has_32fpr 0 >> +#define cpu_has_counter 1 >> +#define cpu_has_watch 1 >> +#define cpu_has_divec 1 >> + >> +#define cpu_has_prefetch 1 >> +#define cpu_has_ejtag 1 >> +#define cpu_has_llsc 1 >> + >> +#define cpu_has_mips16 0 >> +#define cpu_has_mdmx 0 >> +#define cpu_has_mips3d 0 >> +#define cpu_has_smartmips 0 >> +#define cpu_has_mmips 0 >> +#define cpu_has_vz 0 >> + >> +#define cpu_has_mips32r1 1 >> +#define cpu_has_mips32r2 1 >> +#define cpu_has_mips64r1 0 >> +#define cpu_has_mips64r2 0 >> + >> +#define cpu_has_dsp 1 >> +#define cpu_has_dsp2 0 >> +#define cpu_has_mipsmt 1 >> + >> +#define cpu_has_vint 1 >> +#define cpu_has_veic 0 >> + >> +#define cpu_has_64bits 0 >> +#define cpu_has_64bit_zero_reg 0 >> +#define cpu_has_64bit_gp_regs 0 >> +#define cpu_has_64bit_addresses 0 >> + >> +#define cpu_has_cm2 1 >> +#define cpu_has_cm2_l2sync 1 >> +#define cpu_has_eva 1 >> +#define cpu_has_tlbinv 1 >> + >> +#define cpu_dcache_line_size() 32 >> +#define cpu_icache_line_size() 32 >> +#define cpu_scache_line_size() 32 > If you rebase atop linux-next or mips-next then you should find that > many of these defines are now redundant, especially after removing the > SYS_HAS_CPU_MIPS32_R1 select which means your kernel build will always > target MIPS32r2. > > Due to architectural restrictions on where various options can be > present, you should be able to remove: > > - cpu_has_4kex > - cpu_has_3k_cache > - cpu_has_4k_cache > - cpu_has_32fpr > - cpu_has_divec > - cpu_has_prefetch > - cpu_has_llsc > > cpu_has_mmips defaults to a compile-time zero unless you select > CONFIG_SYS_SUPPORTS_MICROMIPS, so please remove that one. > > cpu_has_64bit_gp_regs & cpu_has_64bit_addresses both default to zero > already for 32bit kernel builds, so please remove those. > > cpu_has_cm2 & cpu_has_cm2_l2sync don't exist anywhere in-tree, so please > remove those. > > Additionally cpu_has_sb1_cache is not used anywhere, or defined by > asm/cpu-features.h & seems to just be left over in some platform > override files including presumably one you based yours on. Please > remove it too. Thanks, will remove.
> Also you select CPU_MIPSR2_IRQ_EI but define cpu_has_veic as 0, could > you check that mismatch? The hardware does support, but the software does not support.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/irq.h b/arch/mips/include/asm/mach-intel-mips/irq.h >> new file mode 100644 >> index 000000000000..12a949084856 >> --- /dev/null >> +++ b/arch/mips/include/asm/mach-intel-mips/irq.h >> @@ -0,0 +1,17 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (C) 2014 Lei Chuanhua <Chuanhua.lei@lantiq.com> >> + * Copyright (C) 2018 Intel Corporation. >> + */ >> + >> +#ifndef __INTEL_MIPS_IRQ_H >> +#define __INTEL_MIPS_IRQ_H >> + >> +#define MIPS_CPU_IRQ_BASE 0 >> +#define MIPS_GIC_IRQ_BASE (MIPS_CPU_IRQ_BASE + 8) > These 2 defines are the defaults anyway, so please remove them. Thanks, will remove. >> +#define NR_IRQS 256 > And if you don't actually need this then you could remove irq.h entirely > - do you actually use more than 128 IRQs? Yes, the hardware support 256 IRQs.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/spaces.h b/arch/mips/include/asm/mach-intel-mips/spaces.h >> new file mode 100644 >> index 000000000000..80e7b09f712c >> --- /dev/null >> +++ b/arch/mips/include/asm/mach-intel-mips/spaces.h >> % >% >% >> +#define IO_SIZE _AC(0x10000000, UL) >> +#define IO_SHIFT _AC(0x10000000, UL) > These IO_ defines don't appear to be used anywhere? Thanks, will remove.
>> +/* IO space one */ >> +#define __pa_symbol(x) __pa(x) > Can you explain why you need this, rather than the default definition of > __pa_symbol()? The comment doesn't seem to describe much of anything. Thanks, will remove.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/war.h b/arch/mips/include/asm/mach-intel-mips/war.h >> new file mode 100644 >> index 000000000000..1c95553151e1 >> --- /dev/null >> +++ b/arch/mips/include/asm/mach-intel-mips/war.h >> @@ -0,0 +1,18 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ASM_MIPS_MACH_INTEL_MIPS_WAR_H >> +#define __ASM_MIPS_MACH_INTEL_MIPS_WAR_H >> + >> +#define R4600_V1_INDEX_ICACHEOP_WAR 0 >> +#define R4600_V1_HIT_CACHEOP_WAR 0 >> +#define R4600_V2_HIT_CACHEOP_WAR 0 >> +#define R5432_CP0_INTERRUPT_WAR 0 >> +#define BCM1250_M3_WAR 0 >> +#define SIBYTE_1956_WAR 0 >> +#define MIPS4K_ICACHE_REFILL_WAR 0 >> +#define MIPS_CACHE_SYNC_WAR 0 >> +#define TX49XX_ICACHE_INDEX_INV_WAR 0 >> +#define ICACHE_REFILLS_WORKAROUND_WAR 0 >> +#define R10000_LLSC_WAR 0 >> +#define MIPS34K_MISSED_ITLB_WAR 0 >> + >> +#endif /* __ASM_MIPS_MACH_INTEL_MIPS_WAR_H */ > Since you need none of these workarounds, you shouldn't need war.h at > all. Thanks, will remove this file.
>> diff --git a/arch/mips/intel-mips/Kconfig b/arch/mips/intel-mips/Kconfig >> new file mode 100644 >> index 000000000000..35d2ae2b5408 >> --- /dev/null >> +++ b/arch/mips/intel-mips/Kconfig >> @@ -0,0 +1,22 @@ >> +if INTEL_MIPS >> + >> +choice >> + prompt "Built-in device tree" >> + help >> + Legacy bootloaders do not pass a DTB pointer to the kernel, so >> + if a "wrapper" is not being used, the kernel will need to include >> + a device tree that matches the target board. >> + >> + The builtin DTB will only be used if the firmware does not supply >> + a valid DTB. >> + >> +config DTB_INTEL_MIPS_NONE >> + bool "None" >> + >> +config DTB_INTEL_MIPS_GRX500 >> + bool "Intel MIPS GRX500 Board" >> + select BUILTIN_DTB >> + >> +endchoice >> + >> +endif > So do you actually have both styles of bootloader? this patch only support the build-in, will do a clean up.
>> diff --git a/arch/mips/intel-mips/prom.c b/arch/mips/intel-mips/prom.c >> new file mode 100644 >> index 000000000000..a1b1393c13bc >> --- /dev/null >> +++ b/arch/mips/intel-mips/prom.c >> % >% >% >> +static void __init prom_init_cmdline(void) >> +{ >> + int i; >> + int argc; >> + char **argv; >> + >> + /* >> + * If u-boot pass parameters, it is ok, however, if without u-boot >> + * JTAG or other tool has to reset all register value before it goes >> + * emulation most likely belongs to this category >> + */ >> + if (fw_arg0 == 0 || fw_arg1 == 0) >> + return; > I don't understand what you're trying to say here, or why this check > exists. If you boot with fw_arg0 == fw_arg1 == 0 then you'd just hit the > loop below right, and execute zero iterations of it? That seems like it > would be fine without this special case. this patch do not support this , will remove.
>> + /* >> + * a0: fw_arg0 - the number of string in init cmdline >> + * a1: fw_arg1 - the address of string in init cmdline >> + * >> + * In accordance with the MIPS UHI specification, >> + * the bootloader can pass the following arguments to the kernel: >> + * - $a0: -2. >> + * - $a1: KSEG0 address of the flattened device-tree blob. >> + */ >> + if (fw_arg0 == -2) >> + return; >> + >> + argc = fw_arg0; >> + argv = (char **)KSEG1ADDR(fw_arg1); >> + >> + arcs_cmdline[0] = '\0'; >> + >> + for (i = 0; i < argc; i++) { >> + char *p = (char *)KSEG1ADDR(argv[i]); >> + >> + if (argv[i] && *p) { >> + strlcat(arcs_cmdline, p, sizeof(arcs_cmdline)); >> + strlcat(arcs_cmdline, " ", sizeof(arcs_cmdline)); >> + } >> + } > Why do you need to use kseg1? Why can't the arguments be accessed > cached? > > Are the arguments passed as virtual or physical addresses? If virtual & > we can access them cached then you could replace all of this with a call > to fw_init_cmdline(). this patch only support build-in dts, will remove .
>> +static int __init plat_enable_iocoherency(void) >> +{ >> + if (!mips_cps_numiocu(0)) >> + return 0; >> + >> + /* Nothing special needs to be done to enable coherency */ >> + pr_info("Coherence Manager IOCU detected\n"); >> + /* Second IOCU for MPE or other master access register */ >> + write_gcr_reg0_base(0xa0000000); >> + write_gcr_reg0_mask(0xf8000000 | CM_GCR_REGn_MASK_CMTGT_IOCU1); >> + return 1; >> +} >> + >> +static void __init plat_setup_iocoherency(void) >> +{ >> + if (plat_enable_iocoherency() && >> + coherentio == IO_COHERENCE_DISABLED) { >> + pr_info("Hardware DMA cache coherency disabled\n"); >> + return; >> + } >> + panic("This kind of IO coherency is not supported!"); >> +} > Since you select CONFIG_DMA_NONCOHERENT in Kconfig, coherentio will > always equal IO_COHERENCE_DISABLED. That suggests to me that the above 2 > functions are probably useless, or at least needlessly convoluted. Thanks, will remove.
>> +static int __init plat_publish_devices(void) >> +{ >> + if (!of_have_populated_dt()) >> + return 0; >> + return of_platform_populate(NULL, of_default_bus_match_table, NULL, >> + NULL); >> +} >> +arch_initcall(plat_publish_devices); > The core DT code calls of_platform_populate() already (see > of_platform_default_populate_init()), so you can remove this function. > > Thanks, > Paul Thanks, will remove.
|  |