Date:   Fri, 3 Aug 2018 10:49:24 -0700
From:   Paul Burton <paul.burton@...s.com>
To:     Songjun Wu <songjun.wu@...ux.intel.com>
Cc:     hua.ma@...ux.intel.com, 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

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.

> 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.

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?

> 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.

> 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.

Also you select CPU_MIPSR2_IRQ_EI but define cpu_has_veic as 0, could
you check that mismatch?

> 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.

> +#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?

> 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?

> +/* 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.

> 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.

> 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?

> 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.

> +	/*
> +	 * 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().

> +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.

> +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

Powered by blists - more mailing lists