Hi Bin,
On 1 February 2015 at 19:16, Bin Meng <bmeng...@gmail.com> wrote:
> Hi Simon,
>
> On Mon, Feb 2, 2015 at 12:30 AM, Simon Glass <s...@chromium.org> wrote:
>> Hi Bin,
>>
>> On 29 January 2015 at 02:18, Bin Meng <bmeng...@gmail.com> wrote:
>>> Add minimum codes to support Intel Quark SoC. DRAM initialization
>>> is not ready yet so a hardcoded gd->ram_size is assigned.
>>>
>>> Signed-off-by: Bin Meng <bmeng...@gmail.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Use machine-specific
>>> - Move vairous components' base addresses within Quark SoC to Kconfig
>>> - Rebase to u-boot-86/master
>>>
>>
>> All looks OK but for a few nits.
>>
>>> arch/x86/cpu/quark/Kconfig | 118
>>> +++++++++++++++++++++++++++++++++
>>> arch/x86/cpu/quark/Makefile | 8 +++
>>> arch/x86/cpu/quark/dram.c | 39 +++++++++++
>>> arch/x86/cpu/quark/pci.c | 70 +++++++++++++++++++
>>> arch/x86/cpu/quark/quark.c | 44 ++++++++++++
>>> arch/x86/include/asm/arch-quark/gpio.h | 13 ++++
>>> 6 files changed, 292 insertions(+)
>>> create mode 100644 arch/x86/cpu/quark/Kconfig
>>> create mode 100644 arch/x86/cpu/quark/Makefile
>>> create mode 100644 arch/x86/cpu/quark/dram.c
>>> create mode 100644 arch/x86/cpu/quark/pci.c
>>> create mode 100644 arch/x86/cpu/quark/quark.c
>>> create mode 100644 arch/x86/include/asm/arch-quark/gpio.h
>>>
>>> diff --git a/arch/x86/cpu/quark/Kconfig b/arch/x86/cpu/quark/Kconfig
>>> new file mode 100644
>>> index 0000000..8bccf09
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/quark/Kconfig
>>> @@ -0,0 +1,118 @@
>>> +#
>>> +# Copyright (C) 2015, Bin Meng <bmeng...@gmail.com>
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0+
>>> +#
>>> +
>>> +config INTEL_QUARK
>>> + bool
>>> + select HAVE_RMU
>>> +
>>> +if INTEL_QUARK
>>> +
>>> +config HAVE_RMU
>>> + bool "Add a Remote Management Unit (RMU) binary"
>>> + help
>>> + Select this option to add a Remote Management Unit (RMU) binary
>>> + to the resulting U-Boot image. It is a data block (up to 64K) of
>>> + machine-specific code which must be put in the flash for the RMU
>>> + within the Quark SoC processor to access when powered up before
>>> + system BIOS is executed.
>>> +
>>> +config RMU_FILE
>>> + string "Remote Management Unit (RMU) binary filename"
>>> + depends on HAVE_RMU
>>> + default "rmu.bin"
>>> + help
>>> + The filename of the file to use as Remote Management Unit (RMU)
>>> + binary in the board directory.
>>> +
>>> +config RMU_ADDR
>>> + hex "Remote Management Unit (RMU) binary location"
>>> + depends on HAVE_RMU
>>> + default 0xfff00000
>>> + help
>>> + The location of the RMU binary is determined by a strap. It must
>>> be
>>> + put in flash at a location matching the strap-determined base
>>> address.
>>> +
>>> + The default base address of 0xfff00000 indicates that the binary
>>> must
>>> + be located at offset 0 from the beginning of a 1MB flash device.
>>> +
>>> +config HAVE_CMC
>>> + bool
>>> + default HAVE_RMU
>>> +
>>> +config CMC_FILE
>>> + string
>>> + depends on HAVE_CMC
>>> + default RMU_FILE
>>> +
>>> +config CMC_ADDR
>>> + hex
>>> + depends on HAVE_CMC
>>> + default RMU_ADDR
>>> +
>>> +config ESRAM_BASE
>>> + hex
>>> + default 0x80000000
>>> + help
>>> + Embedded SRAM (eSRAM) memory-mapped base address.
>>> +
>>> +config PCIE_ECAM_BASE
>>> + hex
>>> + default 0xe0000000
>>> +
>>> +config RCBA_BASE
>>> + hex
>>> + default 0xfed1c000
>>> + help
>>> + Root Complex register block memory-mapped base address.
>>> +
>>> +config ACPI_PM1_BASE
>>> + hex
>>> + default 0x1000
>>> + help
>>> + ACPI PM1 i/o-mapped base address.
>>
>> What is PM1?
>
> This is defined in ACPI spec. Given U-Boot does not support ACPI yet,
> and its full description is documented in the ACPI spec, I don't think
> we need describe what it actually does here as this help is not a user
> visible Kconfig option.
OK, but perhaps you can put the abbreviated names in the help?
>
>>> +
>>> +config ACPI_P_BASE
>>> + hex
>>> + default 0x1010
>>> + help
>>> + ACPI PBLK i/o-mapped base address.
>>
>> What is PBLK?
>
> Defined in ACPI spec. Maybe I should define this as ACPI_PBLK_BASE.
>
>>> +
>>> +config SPI_DMA_BASE
>>> + hex
>>> + default 0x1020
>>> + help
>>> + SPI DMA i/o-mapped base address.
>>> +
>>> +config GPIO_BASE
>>> + hex
>>> + default 0x1080
>>> + help
>>> + GPIO i/o-mapped base address.
>>> +
>>> +config GPE0_BASE
>>> + hex
>>> + default 0x1100
>>> + help
>>> + GPE0 i/o-mapped base address.
>>
>> What is GPE0?
>
> Defined in ACPI spec. Maybe I should describe it as "ACPI GPE0", like above
> two.
Whatever you think is fine for the option, but please explain lots in
the help message.
>
>>> +
>>> +config WDT_BASE
>>> + hex
>>> + default 0x1140
>>> + help
>>> + Watchdog timer i/o-mapped base address.
>>> +
>>> +config SYS_CAR_ADDR
>>> + hex
>>> + default ESRAM_BASE
>>> +
>>> +config SYS_CAR_SIZE
>>> + hex
>>> + default 0x8000
>>> + help
>>> + Space in bytes in eSRAM used as Cache-As-ARM (CAR).
>>> + Note this size must not exceed eSRAM's total size.
>>> +
>>> +endif
>>> diff --git a/arch/x86/cpu/quark/Makefile b/arch/x86/cpu/quark/Makefile
>>> new file mode 100644
>>> index 0000000..168c1e6
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/quark/Makefile
>>> @@ -0,0 +1,8 @@
>>> +#
>>> +# Copyright (C) 2015, Bin Meng <bmeng...@gmail.com>
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0+
>>> +#
>>> +
>>> +obj-y += car.o dram.o msg_port.o quark.o
>>> +obj-$(CONFIG_PCI) += pci.o
>>> diff --git a/arch/x86/cpu/quark/dram.c b/arch/x86/cpu/quark/dram.c
>>> new file mode 100644
>>> index 0000000..fbdc3cd
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/quark/dram.c
>>> @@ -0,0 +1,39 @@
>>> +/*
>>> + * Copyright (C) 2015, Bin Meng <bmeng...@gmail.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <asm/post.h>
>>> +#include <asm/arch/quark.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +int dram_init(void)
>>> +{
>>> + /* hardcode the DRAM size for now */
>>> + gd->ram_size = DRAM_MAX_SIZE;
>>> + post_code(POST_DRAM);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +void dram_init_banksize(void)
>>> +{
>>> + gd->bd->bi_dram[0].start = 0;
>>> + gd->bd->bi_dram[0].size = gd->ram_size;
>>> +}
>>> +
>>> +/*
>>> + * This function looks for the highest region of memory lower than 4GB
>>> which
>>> + * has enough space for U-Boot where U-Boot is aligned on a page boundary.
>>> + * It overrides the default implementation found elsewhere which simply
>>> + * picks the end of ram, wherever that may be. The location of the stack,
>>> + * the relocation address, and how far U-Boot is moved by relocation are
>>> + * set in the global data structure.
>>
>> This comment probably doesn't make sense here.
>>
>
> This comment was taken from other codes. So far since we don't have
> MRC, it returns a hardcoded number. Do you think I need change this
> for this patch and when we support MRC we can add this comment back?
Ah I see. Well it's fine to leave as it is then.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
