[U-Boot] [PATCH v4 3/8] mips: add base support for atheros ath79 based SOCs

[Daniel Schwierzeck <daniel.schwierzeck@gmail.com>]

Am 26.12.2015 um 17:17 schrieb Wills Wang:
> 
> 
> On 12/26/2015 03:28 PM, Marek Vasut wrote:
>> On Friday, December 25, 2015 at 07:56:23 PM, Wills Wang wrote:
>>> This patch enable work for ar933x SOC, tested on ar9331 board.
>>>
>>> Signed-off-by: Wills Wang <wills.wang@live.com>
>>> ---
>> [...]
>>
>>> +int arch_cpu_init(void)
>>> +{
>>> +    u32 val;
>>> +
>>> +    /*
>>> +     * Set GPIO10 (UART_SO) as output and enable UART,
>>> +     * BIT(15) in GPIO_FUNCTION_1 register must be written with 1
>>> +     */
>>> +    val = readl(KSEG1ADDR(AR71XX_GPIO_BASE + AR71XX_GPIO_REG_OE));
>>> +    val |= BIT(10);
>>> +    writel(val, KSEG1ADDR(AR71XX_GPIO_BASE + AR71XX_GPIO_REG_OE));
>>> +
>>> +    val = readl(KSEG1ADDR(AR71XX_GPIO_BASE + AR71XX_GPIO_REG_FUNC));
>>> +    val |= (AR933X_GPIO_FUNC_UART_EN | BIT(15));
>>> +    writel(val, KSEG1ADDR(AR71XX_GPIO_BASE + AR71XX_GPIO_REG_FUNC));
>>> +    return 0;
>>> +}
>> Pinmux should be done on a per-board basis, not per-CPU. Also, please use
>> setbits_le32().
> I don't find other more fit entry for pinctrl initialization.
> We must set these IO pin before serial initialization.
> Put it into board directory?

you could use the board_early_init_f hook, implemented in
board/ath79/ap121/ap121.c. If possible you should add a real pinctrl
driver in a later step

>>> diff --git a/arch/mips/mach-ath79/ar933x/ddr_tap.S
>>> b/arch/mips/mach-ath79/ar933x/ddr_tap.S new file mode 100644
>>> index 0000000..18c57de
>>> --- /dev/null
>>> +++ b/arch/mips/mach-ath79/ar933x/ddr_tap.S
>>> @@ -0,0 +1,268 @@
>>> +/*
>>> + * (C) Copyright 2015
>>> + * Wills Wang, <wills.wang@live.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + */
>>> +
>>> +#include <config.h>
>>> +#include <asm/asm.h>
>>> +#include <asm/regdef.h>
>>> +#include <asm/mipsregs.h>
>>> +#include <asm/addrspace.h>
>>> +#include <asm/arch/ar71xx_regs.h>
>>> +
>>> +#define DRAM_K0(x)      KSEG0ADDR(x)
>>> +#define DRAM_K1(x)      KSEG1ADDR(x)
>>> +
>>> +    .text
>>> +    .set noreorder
>>> +
>>> +LEAF(ddr_tap_init)
>>> +    /* Tap settings for the DDR */
>>> +    li      t0, 0xffffffff
>>> +    li      t1, DRAM_K0(0x500000)
>>> +    sw      t0, 0x0(t1)
>>> +    sw      t0, 0x4(t1)
>>> +    sw      t0, 0x8(t1)
>>> +    sw      t0, 0xc(t1)
>>> +    nop
>>> +    nop
>> This should be C code, pretty please.
> Must this be change to c code?
> I think there should be no problem.

this function is only called from init_dram() which already runs in a
full C environment. The conversion to C should be simple. But I would
accept the current patch for the initial merge if you do the conversion
later.

>>
>> [...]
>>
>>> diff --git a/arch/mips/mach-ath79/ar933x/lowlevel_init.S
>>> b/arch/mips/mach-ath79/ar933x/lowlevel_init.S new file mode 100644
>>> index 0000000..72509ca
>>> --- /dev/null
>>> +++ b/arch/mips/mach-ath79/ar933x/lowlevel_init.S
>> lowlevel_init.S should be C code too, I don't see anything which would
>> require this to be ASM .
> I don't find SRAM in this chip, we need DDR memory to handle C runtime
> statck.

currently lowlevel_init() must be coded in assembly because it runs
before RAM and caches are initialized. This function only exists to do
that initialization. MIPS did this from the beginning. Maybe I will send
some patches to use locked cache lines, but that should not be a hard
requirement for adding new SoC's. Actually all modern MIPS based SoC's
have some type of SRAM or first-stage bootloaders, thus adding the cache
line lock feature was not really necessary. I would accept the current
patch for the initial merge.

>> [...]
>>
>>> diff --git a/arch/mips/mach-ath79/cpu.c b/arch/mips/mach-ath79/cpu.c
>>> new file mode 100644
>>> index 0000000..681127c
>>> --- /dev/null
>>> +++ b/arch/mips/mach-ath79/cpu.c
>>> @@ -0,0 +1,171 @@
>>> +/*
>>> + * (C) Copyright 2015
>>> + * Wills Wang, <wills.wang@live.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <asm/io.h>
>>> +#include <asm/addrspace.h>
>>> +#include <asm/types.h>
>>> +#include <asm/arch/ath79.h>
>>> +#include <asm/arch/ar71xx_regs.h>
>>> +
>>> +int print_cpuinfo(void)
>>> +{
>>> +    enum ath79_soc_type soc = ATH79_SOC_UNKNOWN;
>>> +    char *chip = "????";

const char *chip

>>> +    u32 id, major, minor;
>>> +    u32 rev = 0;
>>> +    u32 ver = 1;
>>> +
>>> +    id = readl(KSEG1ADDR(AR71XX_RESET_BASE + AR71XX_RESET_REG_REV_ID));
>>> +    major = id & REV_ID_MAJOR_MASK;
>>> +
>>> +    switch (major) {
>>> +    case REV_ID_MAJOR_AR71XX:
>>> +        minor = id & AR71XX_REV_ID_MINOR_MASK;
>>> +        rev = id >> AR71XX_REV_ID_REVISION_SHIFT;
>>> +        rev &= AR71XX_REV_ID_REVISION_MASK;
>>> +        switch (minor) {
>> I did review this already and my suggestions were ignored :-( I stop
>> here ...
> Sorry, I forget this.
> This code inherit from kernel, to change would maintain it much more
> difficult.
>>

I agree with Marek that a lookup table would be better. But why do you
want to add all possible QCA based SoC's? Until there is no supported
board for other SoC's than yours, this would be dead code.

-- 
- Daniel