| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 14 Jun 2018 17:24:35 +0800
From: yixin zhu <yixin.zhu@...ux.intel.com>
To: James Hogan <jhogan@...nel.org>,
Songjun Wu <songjun.wu@...ux.intel.com>
Cc: hua.ma@...ux.intel.com, chuanhua.lei@...ux.intel.com,
linux-mips@...ux-mips.org, qi-ming.wu@...el.com,
linux-clk@...r.kernel.org, linux-serial@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Rob Herring <robh+dt@...nel.org>,
Ralf Baechle <ralf@...ux-mips.org>,
Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 3/7] MIPS: intel: Add initial support for Intel MIPS SoCs
On 6/12/2018 7:23 PM, James Hogan wrote:
> Hi,
>
> Good to see this patch!
>
> On Tue, Jun 12, 2018 at 01:40:30PM +0800, Songjun Wu wrote:
>> 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
>
> What are the main things preventing this from moving to the generic
> platform? Is it mainly the use of EVA (which generic doesn't yet
> support)?
>
Yes. It's mainly because of EVA.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/kernel-entry-init.h b/arch/mips/include/asm/mach-intel-mips/kernel-entry-init.h
>> new file mode 100644
>> index 000000000000..3893855b60c6
>> --- /dev/null
>> +++ b/arch/mips/include/asm/mach-intel-mips/kernel-entry-init.h
> ...
>> + /*
>> + * Get Config.K0 value and use it to program
>> + * the segmentation registers
>
> Please can you describe (maybe with a table) the segment layout in human
> readable terms so the reader doesn't need to decode the SegCtl registers
> to understand where everything is in the virtual address space?
>
Will add detailed EVA mapping description in code comments.
>> diff --git a/arch/mips/boot/dts/intel-mips/Makefile b/arch/mips/boot/dts/intel-mips/Makefile
>> new file mode 100644
>> index 000000000000..b16c0081639c
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/intel-mips/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +dtb-$(CONFIG_DTB_INTEL_MIPS_GRX500) += easy350_anywan.dtb
>> +obj-y += $(patsubst %.dtb, %.dtb.o, $(dtb-y))
>
> This needs updating to obj-$(CONFIG_BUILTIN_DTB) as per commit
> fca3aa166422 ("MIPS: dts: Avoid unneeded built-in.a in DTS dirs") in
> linux-next.
>
>> diff --git a/arch/mips/intel-mips/Makefile b/arch/mips/intel-mips/Makefile
>> new file mode 100644
>> index 000000000000..9f272d06eecd
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_INTEL_MIPS) += prom.o irq.o time.o
>
> You can use obj-y, since this Makefile is only included if
> CONFIG_INTEL_MIPS=y (i.e. via the platform-$(CONFIG_INTEL_MIPS) below).
>
> Also please split each file onto separate "obj-y += whatever.o" lines.
>
Will use obj-y.
Will split it into per line per .o.
>> diff --git a/arch/mips/intel-mips/Platform b/arch/mips/intel-mips/Platform
>> new file mode 100644
>> index 000000000000..b34750eeaeb0
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/Platform
>> @@ -0,0 +1,11 @@
>> +#
>> +# MIPs SoC platform
>> +#
>> +
>> +platform-$(CONFIG_INTEL_MIPS) += intel-mips/
>
> ^^^ (this is what ensures the Makefile is only included for this
> platform)
>
>> diff --git a/arch/mips/intel-mips/irq.c b/arch/mips/intel-mips/irq.c
>> new file mode 100644
>> index 000000000000..00637a5cdd20
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/irq.c
>> @@ -0,0 +1,36 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2016 Intel Corporation.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of_irq.h>
>> +#include <asm/irq.h>
>> +
>> +#include <asm/irq_cpu.h>
>> +
>> +void __init arch_init_irq(void)
>> +{
>> + struct device_node *intc_node;
>> +
>> + pr_info("EIC is %s\n", cpu_has_veic ? "on" : "off");
>> + pr_info("VINT is %s\n", cpu_has_vint ? "on" : "off");
>> +
>> + intc_node = of_find_compatible_node(NULL, NULL,
>> + "mti,cpu-interrupt-controller");
>> + if (!cpu_has_veic && !intc_node)
>> + mips_cpu_irq_init();
>> +
>> + irqchip_init();
>> +}
>> +
>> +int get_c0_perfcount_int(void)
>> +{
>> + return gic_get_c0_perfcount_int();
>> +}
>> +EXPORT_SYMBOL_GPL(get_c0_perfcount_int);
>> +
>> +unsigned int get_c0_compare_int(void)
>> +{
>> + return gic_get_c0_compare_int();
>> +}
>
> Worth having get_c0_fdc_int() too for the "Fast Debug Channel"?
>
FDC not used in our product.
>> diff --git a/arch/mips/intel-mips/prom.c b/arch/mips/intel-mips/prom.c
>> new file mode 100644
>> index 000000000000..9407858ddc94
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/prom.c
>> @@ -0,0 +1,184 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2014 Lei Chuanhua <Chuanhua.lei@...tiq.com>
>> + * Copyright (C) 2016 Intel Corporation.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/export.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <asm/mips-cps.h>
>> +#include <asm/smp-ops.h>
>> +#include <asm/dma-coherence.h>
>> +#include <asm/prom.h>
>> +
>> +#define IOPORT_RESOURCE_START 0x10000000
>> +#define IOPORT_RESOURCE_END 0xffffffff
>> +#define IOMEM_RESOURCE_START 0x10000000
>> +#define IOMEM_RESOURCE_END 0xffffffff
>
> The _END ones seem to be unused?
>
Will remove unused _END macros.
>> +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;
>> +
>> + 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));
>> + }
>> + }
>
> Please describe the boot register protocol in the commit message and/or
> a comment in this function.
>Will add boot register protocol description in code comment.
> Is it compatible with the UHI boot protocol, such that this could
> potentially be converted to use the generic platform in future?
>
It is compatible with the UHI boot protocol.
>> +}
>> +
>> +static int __init plat_enable_iocoherency(void)
>> +{
>> + int supported = 0;
>> +
>> + if (mips_cps_numiocu(0) != 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);
>> + supported = 1;
>> + }
>> +
>> + /* hw_coherentio = supported; */
>> +
>> + return supported;
>> +}
>> +
>> +static void __init plat_setup_iocoherency(void)
>> +{
>> +#ifdef CONFIG_DMA_NONCOHERENT
>> + /*
>> + * Kernel has been configured with software coherency
>> + * but we might choose to turn it off and use hardware
>> + * coherency instead.
>
> That sounds a big risky. Software coherency will I think perform cache
> line invalidation when syncing buffers from device to CPU (see
> __dma_sync_virtual), so that the underlying RAM written by the
> supposedly incoherent DMA is visible. If its coherent afterall then it
> may be sat in a dirty line in the cache, and not have been written back
> to RAM yet before it gets invalidated?
>
The comment in code is not accurate.
HW coherence is not supported and software coherence is always enabled.
Will correct the comments and clean the code.
>> + */
>> + if (plat_enable_iocoherency()) {
>> + if (coherentio == IO_COHERENCE_DISABLED)
>> + pr_info("Hardware DMA cache coherency disabled\n");
>> + else
>> + pr_info("Hardware DMA cache coherency enabled\n");
>> + } else {
>> + if (coherentio == IO_COHERENCE_ENABLED)
>> + pr_info("Hardware DMA cache coherency unsupported, but enabled from command line!\n");
>> + else
>> + pr_info("Software DMA cache coherency enabled\n");
>> + }
>> +#else
>> + if (!plat_enable_iocoherency())
>> + panic("Hardware DMA cache coherency not supported!");
>> +#endif
>> +}
>
>
>> +void __init device_tree_init(void)
>> +{
>> + if (!initial_boot_params)
>> + return;
>
> Redundant check. unflatten_and_copy_device_tree() now handles that and
> emits a message.
>
Will remove the redundant check.
>> +
>> + unflatten_and_copy_device_tree();
>> +}
>> +
>> +#define CPC_BASE_ADDR 0x12310000
>
> Please put this at the top of the file with other #defines.
>
Will move this to the top with other #defines.
>> +
>> +phys_addr_t mips_cpc_default_phys_base(void)
>> +{
>> + return CPC_BASE_ADDR;
>> +}
>
>> diff --git a/arch/mips/intel-mips/time.c b/arch/mips/intel-mips/time.c
>> new file mode 100644
>> index 000000000000..77ad4014fe9d
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/time.c
>> @@ -0,0 +1,56 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2016 Intel Corporation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/of.h>
>> +
>> +#include <asm/time.h>
>> +
>> +static inline u32 get_counter_resolution(void)
>> +{
>> + u32 res;
>> +
>> + __asm__ __volatile__(".set push\n"
>
> Preferably each line of asm should end \n\t and the final line doesn't
> need \n or \t. Look at the .s compiler output to see the difference.
>
>> + ".set mips32r2\n"
>> + "rdhwr %0, $3\n"
>
> Hmm, it'd be nice to abstract this in mipsregs.h, but that can always
> wait. You could use MIPS_HWR_CCRES though (i.e. off top of my head
> something like "%0, $%1\n" and have a "i" (MIPS_HWR_CCRES) input.
>
>> + ".set pop\n"
>> + : "=&r" (res)
>
> I don't think you strictly need an early clobber there since there are
> no register inputs, "=r" should do fine.
>
>> + : /* no input */
>> + : "memory");
>
> I don't think that operation clobbers any memory?
>
Will replace the assembly code with fixed value 2 as the chip resolution
is the half of the clock.
> Thanks
> James
>
Thanks
Yixin
Powered by blists - more mailing lists