RE: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup

[Ong, Boon Leong]

>Subject: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup
>
[snippet removed]

>Since the Quark EFI bringup code configures the system to reset on an IMR
Typo: bring-up

>violation, this means that common operations such as mouting an SD based root
Typo: mounting

[snippet removed]

>+config INTEL_GALILEO
>+	bool "Intel Galileo Platform Support"
>+	depends on X86_32 && PCI
>+	select IOSF_MBI
>+	select IMR
>+	---help---
>+	  Intel Galileo platform support. This code is used to tear-down
>+	  BIOS and Grub Isolated Memory Regions used during bootup.
Missing dash. Should be "boot-up".

[snippet removed]

>diff --git a/drivers/platform/x86/intel_galileo.c
>b/drivers/platform/x86/intel_galileo.c
>new file mode 100644
>index 0000000..2a555aa
>--- /dev/null
>+++ b/drivers/platform/x86/intel_galileo.c
>@@ -0,0 +1,175 @@
>+/*
>+ * intel_galileo.c - Intel Galileo platform support.
>+ *
>+ * Copyright(c) 2013 Intel Corporation.
>+ * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx>
>+ *
>+ * This platform code provides an entry point to do Galileo specific
>+ * setup. Critically IMRs are policed to ensure the EFI provided memory
Critically --> Critical

>+ * map informing the kernel of it's available memory is consistent with
It's --> its

[snippet removed]

>+
>+enum {
>+	GALILEO_UNKNOWN = 0,
>+	GALILEO_QRK_GEN1,
>+	GALILEO_QRK_GEN2,
>+};
Suggest to drop QRK to kill confusion that it is Quark Gen 1 & 2. 

[snippet removed]

>+#ifdef DEBUG
>+#define SANITY "IMR : sanity error "
>+
>+/**
Per coding style, please just use /* and kill the extra * above and below.

>+ * intel_galileo_imr_sanity
>+ * Verify IMR sanity with some simple tests to verify
>+ * overlap, zero sized allocations
>+ *
>+ * @base: Physical base address of the kernel text section
>+ * @size: Extent of kernel memory to be covered from &_text to
>+&_sinittext
>+ * @return: none
>+ */
>+static void __init
>+intel_galileo_imr_sanity(unsigned long base, unsigned long size) {
>+	/* Test zero zero */
>+	if (imr_add(0, 0, 0, 0, true) == 0)
>+		pr_err(SANITY "zero sized IMR @ 0x00000000\n");

A side-discussion on imr_add():
I would think that we should allow 1KiB IMR setting. Current imr_add() logic
is prohibiting it. So, the 'size' input should be at least 1KiB and imr_add()
internal logic will adjust 'hi' by -1KiB. Please consider ..  
  
>+
>+	/* Test overlap */
>+	if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
>+		pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
>+		       base, base + size);
>+
>+	/* Try overlap - IMR_ALIGN */
>+	base = base + size - IMR_ALIGN;
>+	if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
>+		pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
>+		       base, base + size);
>+}
>+#endif
>+
>+/**
>+ * intel_galileo_imr_init
>+ *
>+ * Tear down IMRs used during bootup. BIOS and Grub
boot-up.

>+ * both setup IMRs around compressed kernel, initrd memory
>+ * that need to be removed before the kernel hands out one
>+ * of the IMR encased addresses to a downstream DMA agent
>+ * such as the SD or Ethernet. IMRs on Galileo are setup to
>+ * immediately reset the system on violation - so if you're
>+ * running a root filesystem from SD - you'll need the IMRs
>+ * torn down or you'll find seemingly random resets when using
>+ * your filesystem.
>+ */
>+static void __init intel_galileo_imr_init(void) {
>+	unsigned long base  = virt_to_phys(&_text);
>+	unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN;
>+	int i, ret;
>+
>+	/* Tear down all existing unlocked IMRs */
>+	for (i = 0; i <= QUARK_X1000_IMR_NUM; i++)
>+		imr_del(i, 0, 0);
>+
>+	/*
>+	 * Setup an IMR around the physical extent of the kernel
>+	 * Non-SMM mode core read/write from/to kernel physical region.
>+	 * IMR locked.
>+	 */
>+	ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true);
>+	if (ret)
>+		pr_err("Unable to setup IMR for kernel: (%p - %p)\n",
>+			&_text, &__init_begin);
>+	else
>+		pr_info("IMR protect kernel memory: %ldk (%p - %p)\n",
>+			size >> 10, &_text, &__init_begin);
Perhaps we want to be consistent between using __init_begin & _sinittext?

>+#ifdef DEBUG
>+	intel_galileo_imr_sanity(base, size);
>+#endif
>+}
>+
>+/**
>+ * intel_galileo_init
>+ *
>+ * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the
>+ * kernel memory space of IMRs that are inconsistent with the EFI memory
>map.
>+ */
>+static int __init intel_galileo_init(void) {
>+	int ret = 0, type = GALILEO_UNKNOWN;
type is assigned to either GALILEO_GEN1|2 below anyway.
So, we don't need to declare to UNKNOWN?

[snippet removed]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/