lkml.org 
[lkml]   [2018]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/7] MIPS: intel: Add initial support for Intel MIPS SoCs
    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)?

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

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

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

    > 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@lantiq.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?

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

    Is it compatible with the UHI boot protocol, such that this could
    potentially be converted to use the generic platform in future?

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

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

    > +
    > + unflatten_and_copy_device_tree();
    > +}
    > +
    > +#define CPC_BASE_ADDR 0x12310000

    Please put this at the top of the file 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?

    Thanks
    James
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-06-12 13:24    [W:4.191 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site