Date:   Mon, 6 Aug 2018 17:12:41 +0800
From:   Hua Ma <hua.ma@...ux.intel.com>
To:     Paul Burton <paul.burton@...s.com>,
        Songjun Wu <songjun.wu@...ux.intel.com>
Cc:     yixin.zhu@...ux.intel.com, chuanhua.lei@...ux.intel.com,
        qi-ming.wu@...el.com, linux-mips@...ux-mips.org,
        linux-clk@...r.kernel.org, linux-serial@...r.kernel.org,
        devicetree@...r.kernel.org, James Hogan <jhogan@...nel.org>,
        linux-kernel@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>
Subject: Re: [PATCH v2 01/18] MIPS: intel: Add initial support for Intel MIPS
 SoCs



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@...ux.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@...tiq.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.

Powered by blists - more mailing lists