[U-Boot] [PATCH v1] cpu: Add Intel Tangier support

[Bin Meng]

Hi Andy,

On Thu, Apr 20, 2017 at 3:23 PM, Andy Shevchenko
<andy.shevchenko at gmail.com> wrote:
> On Thu, Apr 20, 2017 at 5:59 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Andy,
>>
>> On Tue, Apr 18, 2017 at 10:21 PM, Andy Shevchenko
>> <andriy.shevchenko at linux.intel.com> wrote:
>>> From: Felipe Balbi <felipe.balbi at linux.intel.com>
>>>
>>> Add Intel Tangier SoC support.
>>>
>>> Intel Tangier SoC is a core part of Intel Merrifield platform. For
>>> example, Intel Edison board is based on such platform.
>>>
>>> The patch is based on work done by the following people (in alphabetical
>>> order):
>>>         Aiden Park <aiden.park at intel.com>
>>>         Dukjoon Jeon <dukjoon.jeon at intel.com>
>>>         eric.park <eric.park at intel.com>
>>>         Fabien Chereau <fabien.chereau at intel.com>
>>>         Scott D Phillips <scott.d.phillips at intel.com>
>>>         Sebastien Colleur <sebastienx.colleur at intel.com>
>>>         Steve Sakoman <steve.sakoman at intel.com>
>>>         Vincent Tinelli <vincent.tinelli at intel.com>
>
>>> +#include <common.h>
>>> +#include <malloc.h>
>>> +#include <asm/bootparam.h>
>>> +#include <asm/e820.h>
>>> +#include <asm/global_data.h>
>>> +#include <asm/processor.h>
>>> +#include <asm/sections.h>
>>> +#include <asm/sfi.h>
>>> +#include <asm/u-boot-x86.h>
>>
>> Can you double check if all these header files are needed here?
>
> Will do.
>
>>> +/* Memory type definitions */
>>> +enum sfi_mem_type {
>>> +       SFI_MEM_RESERVED,
>>> +       SFI_LOADER_CODE,
>>> +       SFI_LOADER_DATA,
>>> +       SFI_BOOT_SERVICE_CODE,
>>> +       SFI_BOOT_SERVICE_DATA,
>>> +       SFI_RUNTIME_SERVICE_CODE,
>>> +       SFI_RUNTIME_SERVICE_DATA,
>>> +       SFI_MEM_CONV,
>>> +       SFI_MEM_UNUSABLE,
>>> +       SFI_ACPI_RECLAIM,
>>> +       SFI_ACPI_NVS,
>>> +       SFI_MEM_MMIO,
>>> +       SFI_MEM_IOPORT,
>>> +       SFI_PAL_CODE,
>>> +       SFI_MEM_TYPEMAX,
>>> +};
>>> +
>>
>> Should this be moved to sfi.h?
>
> If no one objects, I would do this.
>
>>
>>> +#define SFI_BASE_ADDR          0x000E0000
>>> +#define SFI_LENGTH             0x00020000
>>> +#define SFI_TABLE_LENGTH       16
>>> +
>>
>> Can you add some comments here? I guess U-Boot on tangier is not the
>> 1st stage bootloader. It boots from the 1st stage bootloader and get
>> memory information via SFI table which 1st stage bootloader provides?
>>

Can you confirm if my above comments are correct? If so, please add
some comments.

>
>>> diff --git a/arch/x86/cpu/tangier/tangier.c b/arch/x86/cpu/tangier/tangier.c
>
>>> diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
>>> new file mode 100644
>>> index 0000000000..7de4c08e36
>>> --- /dev/null
>>> +++ b/arch/x86/include/asm/dma-mapping.h
>>> @@ -0,0 +1,41 @@
>>> +/*
>>> + * (C) Copyright 2007
>>> + * Stelian Pop <stelian at popies.net>
>>> + * Lead Tech Design <www.leadtechdesign.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +#ifndef __ASM_X86_DMA_MAPPING_H
>>> +#define __ASM_X86_DMA_MAPPING_H
>>> +
>>> +#define        dma_mapping_error(x, y) 0
>>> +
>>> +enum dma_data_direction {
>>> +       DMA_BIDIRECTIONAL       = 0,
>>> +       DMA_TO_DEVICE           = 1,
>>> +       DMA_FROM_DEVICE         = 2,
>>> +};
>>> +
>>> +static inline void *dma_alloc_coherent(size_t len, unsigned long *handle)
>>> +{
>>> +       *handle = (unsigned long)memalign(ARCH_DMA_MINALIGN, len);
>>> +       return (void *)*handle;
>>> +}
>>> +
>>> +static inline void dma_free_coherent(void *addr)
>>> +{
>>> +       free(addr);
>>> +}
>>> +
>>> +static inline unsigned long dma_map_single(volatile void *vaddr, size_t len,
>>> +                                          enum dma_data_direction dir)
>>> +{
>>> +       return (unsigned long)vaddr;
>>> +}
>>> +
>>> +static inline void dma_unmap_single(volatile void *vaddr, size_t len,
>>> +                                   unsigned long paddr)
>>> +{
>>> +}
>>> +
>>> +#endif /* __ASM_X86_DMA_MAPPING_H */
>>> --
>>
>> Why is this dma-mapping.h file needed? For x86, all memory are
>> coherent, which is indicated in your implementation as well
>> (malloc/free are used)
>
> dwc3 is used on many platforms and SoCs, so, it's irrelevant to x86.
> Other option is to uglify dwc3 driver. I dunno it would be a good idea.
>

OK, so please split this patch into two, Leave the adding of
dma-mapping.h file to a single patch, and state that this is required
on some cross-platform drivers.

Regards,
Bin