[U-Boot] [PATCH 4/4] ARM: atmel: add sama5d3xek support
Andreas Bießmann
andreas.devel at googlemail.com
Fri Mar 8 08:32:12 CET 2013
Hi Bo,
On 08.03.13 04:31, Bo Shen wrote:
> Hi Andreas,
> Thansk for your comments.
>
> On 3/7/2013 18:58, Andreas Bießmann wrote:
>> Dear Bo Shen,
>>
>> On 28.02.13 08:00, Bo Shen wrote:
>>> Add sama5d3xek support with following feature
>>> - boot from NAND flash, PMECC support, 4bit ECC @ 512 bytes sector
>>> - boot from SPI flash support
>>> - boot from SD card support
>>> - LCD support
>>> - EMAC support
>>> - USB support
>>>
>>> Signed-off-by: Bo Shen <voice.shen at atmel.com>
>>> ---
<snip>
>>> diff --git a/arch/arm/cpu/armv7/at91/clock.c
>>> b/arch/arm/cpu/armv7/at91/clock.c
>>> new file mode 100644
>>> index 0000000..5fdefcc
>>> --- /dev/null
>>> +++ b/arch/arm/cpu/armv7/at91/clock.c
>>> @@ -0,0 +1,116 @@
<snip>
>>> +static u32 at91_pll_rate(u32 freq, u32 reg)
>>> +{
>>> + unsigned mul, div;
>>> +
>>> + div = reg & 0xff;
>>> + mul = (reg >> 18) & 0x7ff;
>>
>> The CKGR_PLLAR states that the MULA is 8 bit wide. Is the mask here
>> intended?
>> I see, it is stolen from the arm926ejs variant, has anyone checked that
>> before ;)
>
> at91sam9g20, at91sam9g45, the MULA is 8 bit wide
> And other at91 serials is 11 bit wide.
> sama5d3x, the MULA is only 7 bit wide. I will correct it.
You are right, it is just 7 bit wide (not 8 bit as I said) ...
<snip>
>>> +void at91_periph_clk_enable(int id)
>>> +{
>>> + struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>>
>> The write protect is meant to be right here?
>
> I am sorry, I don't get the right means.
Well, this seems to be the first SoC from Atmel which has write
protection bit WPEN in pmc->wpmr for write access to pmc->pcer{0|1}. My
question here is, should we rely on reset state of the WPEN bit or
should we explicitly disable write protection on every call to
at91_periph_clk_enable() or should we do this conditional?
Additionally we may ask if we should enable write protection afterwards.
<snip>
>>> diff --git a/arch/arm/cpu/armv7/at91/sama5d3_devices.c
>>> b/arch/arm/cpu/armv7/at91/sama5d3_devices.c
>>> new file mode 100644
>>> index 0000000..e5474e0
>>> --- /dev/null
>>> +++ b/arch/arm/cpu/armv7/at91/sama5d3_devices.c
>>> @@ -0,0 +1,219 @@
>>> +/*
>>> + * Copyright (C) 2012-2013 Atmel Corporation
>>
>> Your email here?
>
> OK, I will add it.
>
>>> + *
>>> + * See file CREDITS for list of people who contributed to this
>>> + * project.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation; either version 2 of
>>> + * the License, or (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <asm/arch/sama5d3.h>
>>> +#include <asm/arch/at91_common.h>
>>> +#include <asm/arch/at91_pmc.h>
>>> +#include <asm/arch/clk.h>
>>> +#include <asm/arch/gpio.h>
>>> +#include <asm/io.h>
>>> +
>>> +/*
>>> + * if CONFIG_AT91_GPIO_PULLUP ist set, keep pullups on on all
>>> + * peripheral pins. Good to have if hardware is soldered optionally
>>> + * or in case of SPI no slave is selected. Avoid lines to float
>>> + * needlessly. Use a short local PUP define.
>>> + *
>>> + * Due to errata "TXD floats when CTS is inactive" pullups are always
>>> + * on for TXD pins.
>>> + */
>>> +#ifdef CONFIG_AT91_GPIO_PULLUP
>>> +# define PUP CONFIG_AT91_GPIO_PULLUP
>>> +#else
>>> +# define PUP 0
>>> +#endif
>>> +
<snip>
>>> +
>>> +void at91_serial0_hw_init(void)
>>> +{
>>> + at91_set_a_periph(AT91_PIO_PORTD, 18, 1); /* TXD0 */
>>
>> Why enabling PUP here for TX ...
>
> I will correct it to use PUP.
Don't get me wrong, please do not use the PUP define which is
conditional. We should enable pullup for each TX line _unconditional_
which means place a '1' at last argument for each TX line (also for the
serial1_hw line which didn't had it in this patch). I don't know if the
mentioned errata (in comment above) also hits the sama5, but I think it
is good to have the TX line pulled up even if some external PU exists.
The internal PU is about 100 to 300 kOhms, a typical external PU is
about 10kOhms or 100kOhms, so the resulting resistance is nearly 10kOhm
(for 10k external) or about 50k to 75k, depending on concrete internal
resistance. I think this is ok.
The RX line instead should be conditional with PUP define, how do you
think about?
>>> + at91_set_a_periph(AT91_PIO_PORTD, 17, PUP); /* RXD0 */
>>> +
>>> + /* Enable clock */
>>> + at91_periph_clk_enable(ATMEL_ID_USART0);
>>> +}
>>> +
>>> +void at91_serial1_hw_init(void)
>>> +{
>>> + at91_set_a_periph(AT91_PIO_PORTB, 28, 0); /* TXD1 */
>>
>> ... but not here?
>
> I will add PUP for it.
>
>>
>>> + at91_set_a_periph(AT91_PIO_PORTB, 29, PUP); /* RXD1 */
>>> +
>>> + /* Enable clock */
>>> + at91_periph_clk_enable(ATMEL_ID_USART1);
>>> +}
>>> +
>>> +void at91_serial2_hw_init(void)
>>> +{
>>> + at91_set_b_periph(AT91_PIO_PORTE, 26, 1); /* TXD2 */
>>> + at91_set_b_periph(AT91_PIO_PORTE, 25, PUP); /* RXD2 */
>>> +
>>> + /* Enable clock */
>>> + at91_periph_clk_enable(ATMEL_ID_USART2);
>>> +}
>>> +
>>> +void at91_seriald_hw_init(void)
>>> +{
>>> + at91_set_a_periph(AT91_PIO_PORTB, 30, 0); /* DRXD */
>>> + at91_set_a_periph(AT91_PIO_PORTB, 31, 1); /* DTXD */
>>
>> Can you swap the both lines before? There is always TX before RX but the
>> DBGU is an exception ... please change.
>> Shouldn't the DRXD also have PUP conditionally as the other RX lines
>> have?
>
> OK. I will add this.
>
>>> +
>>> + /* Enable clock */
>>> + at91_periph_clk_enable(ATMEL_ID_SYS);
>>> +}
>>> +
>>> +#if defined(CONFIG_ATMEL_SPI)
>>> +void at91_spi0_hw_init(unsigned long cs_mask)
>>> +{
>>> + at91_set_a_periph(AT91_PIO_PORTD, 10, 0); /* SPI0_MISO */
>>> + at91_set_a_periph(AT91_PIO_PORTD, 11, 0); /* SPI0_MOSI */
>>> + at91_set_a_periph(AT91_PIO_PORTD, 12, 0); /* SPI0_SPCK */
>>> +
>>> + if (cs_mask & (1 << 0))
>>> + at91_set_pio_output(AT91_PIO_PORTD, 13, 0);
>>> + if (cs_mask & (1 << 1))
>>> + at91_set_pio_output(AT91_PIO_PORTD, 14, 0);
>>> + if (cs_mask & (1 << 2))
>>> + at91_set_pio_output(AT91_PIO_PORTD, 15, 0);
>>> + if (cs_mask & (1 << 3))
>>> + at91_set_pio_output(AT91_PIO_PORTD, 16, 0);
>>
>> PUP for the PIO's? The comment above states 'Good to have if hardware is
>> soldered optionally or in case of SPI no slave is selected.' ...
>
> Here, I think, we should set as PIO with pull up. when we need to access
> spi flash, then we active this pin.
You are right, but should we use PUP or set pull up unconditionally?
<snip>
>>> +#ifdef CONFIG_LCD
>>> +void at91_lcd_hw_init(void)
>>> +{
>>
>> Can you place an hint here, that this currently supports only wireing of
>> LCDDx on PORTA up to 16 bit? Or something like 'only sama5d3x board
>> style currently imlemented'.
>
> I am not fully understand about this. why should we put this here?
Well, I checked the PIO lines and found out, that PIOA0 to PIOA29
(without the gap from PIOA15 to PIOA23) is sufficient to drive the 24bit
LCD. On sama5d3xek however they use PORTC and PORTE to drive the LCDD16
to LCDD23. I think it is at least worth an comment. Or we use some
parameter to distinguish between the different setups.
> Maybe we should check LCD_OUTPUT_BPP to set whether need high 8 bit
> (port C configuration)?
I forgot that LCD can be used with just 16bit, so the PORTA up to
PORTA15 would be sufficient. But that can somebody implement when he
needs it. What we need now is some way to differentiate between the
setups. I personally find the avr32 solution good, have a look at
arch/avr32/cpu/at32ap700x/portmux.c:portmux_enable_lcdc(). This however
will not match our case here cause the ap700x has two complete outputs
for one lcdc where the sama5 has only one option for the lcd control
lines and first 16 data lines but two options for the higher data lines.
>>> + at91_set_a_periph(AT91_PIO_PORTA, 24, 0); /* LCDPWM */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 25, 0); /* LCDDISP */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 26, 0); /* LCDVSYNC */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 27, 0); /* LCDHSYNC */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 28, 0); /* LCDDOTCK */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 29, 0); /* LCDDEN */
>>> +
>>> + at91_set_a_periph(AT91_PIO_PORTA, 0, 0); /* LCDD0 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 1, 0); /* LCDD1 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 2, 0); /* LCDD2 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 3, 0); /* LCDD3 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 4, 0); /* LCDD4 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 5, 0); /* LCDD5 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 6, 0); /* LCDD6 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 7, 0); /* LCDD7 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 8, 0); /* LCDD8 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 9, 0); /* LCDD9 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 10, 0); /* LCDD10 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 11, 0); /* LCDD11 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 12, 0); /* LCDD12 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 13, 0); /* LCDD13 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 14, 0); /* LCDD14 */
>>> + at91_set_a_periph(AT91_PIO_PORTA, 15, 0); /* LCDD15 */
>>> + at91_set_c_periph(AT91_PIO_PORTC, 14, 0); /* LCDD16 */
>>> + at91_set_c_periph(AT91_PIO_PORTC, 13, 0); /* LCDD17 */
>>> + at91_set_c_periph(AT91_PIO_PORTC, 12, 0); /* LCDD18 */
>>> + at91_set_c_periph(AT91_PIO_PORTC, 11, 0); /* LCDD19 */
>>> + at91_set_c_periph(AT91_PIO_PORTC, 10, 0); /* LCDD20 */
>>> + at91_set_c_periph(AT91_PIO_PORTC, 15, 0); /* LCDD21 */
>>> + at91_set_c_periph(AT91_PIO_PORTE, 27, 0); /* LCDD22 */
>>> + at91_set_c_periph(AT91_PIO_PORTE, 28, 0); /* LCDD23 */
>>> +
>>> + /* Enable clock */
>>> + at91_periph_clk_enable(ATMEL_ID_LCDC);
>>> +}
>>> +#endif
<snip>
>>> diff --git a/board/atmel/sama5d3xek/sama5d3xek.c
>>> b/board/atmel/sama5d3xek/sama5d3xek.c
>>> new file mode 100644
>>> index 0000000..352d20c
>>> --- /dev/null
>>> +++ b/board/atmel/sama5d3xek/sama5d3xek.c
>>> @@ -0,0 +1,274 @@
>>> +/*
>>> + * Copyright (C) 2012 - 2013 Atmel Corporation
>>
>> your email?
>
> OK, I will add it.
>
>>
>>> + *
>>> + * See file CREDITS for list of people who contributed to this
>>> + * project.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation; either version 2 of
>>> + * the License, or (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <mmc.h>
>>> +#include <asm/io.h>
>>> +#include <asm/arch/sama5d3_smc.h>
>>> +#include <asm/arch/at91_common.h>
>>> +#include <asm/arch/at91_pmc.h>
>>> +#include <asm/arch/at91_rstc.h>
>>> +#include <asm/arch/gpio.h>
>>> +#include <asm/arch/clk.h>
>>> +#include <lcd.h>
>>> +#include <atmel_lcdc.h>
>>> +#include <atmel_mci.h>
>>> +#include <net.h>
>>> +#include <netdev.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +/*
>>> -------------------------------------------------------------------------
>>> */
>>> +/*
>>> + * Miscelaneous platform dependent initialisations
>>> + */
>>> +
>>> +#ifdef CONFIG_CMD_NAND
>>> +void sama5d3xek_nand_hw_init(void)
>>> +{
>>> + struct at91_smc *smc = (struct at91_smc *)ATMEL_BASE_SMC;
>>> +
>>> + at91_periph_clk_enable(ATMEL_ID_SMC);
>>> +
>>> + /* Configure SMC CS3 for NAND/SmartMedia */
>>> + writel(AT91_SMC_SETUP_NWE(2) | AT91_SMC_SETUP_NCS_WR(1) |
>>> + AT91_SMC_SETUP_NRD(2) | AT91_SMC_SETUP_NCS_RD(1),
>>> + &smc->cs[3].setup);
>>> + writel(AT91_SMC_PULSE_NWE(3) | AT91_SMC_PULSE_NCS_WR(5) |
>>> + AT91_SMC_PULSE_NRD(3) | AT91_SMC_PULSE_NCS_RD(5),
>>> + &smc->cs[3].pulse);
>>> + writel(AT91_SMC_CYCLE_NWE(8) | AT91_SMC_CYCLE_NRD(8),
>>> + &smc->cs[3].cycle);
>>> + writel(AT91_SMC_TIMINGS_TCLR(3) | AT91_SMC_TIMINGS_TADL(10) |
>>> + AT91_SMC_TIMINGS_TAR(3) | AT91_SMC_TIMINGS_TRR(4) |
>>> + AT91_SMC_TIMINGS_TWB(5) | AT91_SMC_TIMINGS_RBNSEL(3)|
>>> + AT91_SMC_TIMINGS_NFSEL(1), &smc->cs[3].timings);
>>> + writel(AT91_SMC_MODE_RM_NRD | AT91_SMC_MODE_WM_NWE |
>>> + AT91_SMC_MODE_EXNW_DISABLE |
>>> +#ifdef CONFIG_SYS_NAND_DBW_16
>>> + AT91_SMC_MODE_DBW_16 |
>>> +#else /* CONFIG_SYS_NAND_DBW_8 */
>>> + AT91_SMC_MODE_DBW_8 |
>>> +#endif
>>> + AT91_SMC_MODE_TDF_CYCLE(3),
>>> + &smc->cs[3].mode);
>>> +}
>>> +#endif
>>> +
>>> +#ifdef CONFIG_CMD_USB
>>> +static void sama5d3xek_usb_hw_init(void)
>>> +{
>>> + at91_set_pio_output(AT91_PIO_PORTD, 25, 0);
>>> + at91_set_pio_output(AT91_PIO_PORTD, 26, 0);
>>> + at91_set_pio_output(AT91_PIO_PORTD, 27, 0);
>>> +}
>>> +#endif
>>
>> Isn't the usb stuff generic? Shouldn't we move it to sama5d3_devices.c?
>
> This is board related, used to control the +5V power.
Ouch, ok was my fault.
>
>>> +
>>> +#ifdef CONFIG_GENERIC_ATMEL_MCI
>>> +static void sama5d3xek_mci_hw_init(void)
>>> +{
>>
>> place here the power gpio thing for this board.
>
> OK.
>
>>> + at91_mci_hw_init();
>>> +}
>>> +#endif
>>> +
>>> +#ifdef CONFIG_MACB
>>> +static void sama5d3xek_macb_hw_init(void)
>>> +{
>>
>> remove this function and move the at91_macb_hw_init() down to
>> board_init().
>
> OK.
>
>>> + at91_macb_hw_init();
>>> +}
>>> +#endif
>>> +
>>> +#ifdef CONFIG_LCD
>>> +vidinfo_t panel_info = {
>>> + .vl_col = 800,
>>> + .vl_row = 480,
>>> + .vl_clk = 24000000,
>>> + .vl_sync = ATMEL_LCDC_INVLINE_NORMAL | ATMEL_LCDC_INVFRAME_NORMAL,
>>> + .vl_bpix = LCD_BPP,
>>> + .vl_tft = 1,
>>> + .vl_hsync_len = 128,
>>> + .vl_left_margin = 64,
>>> + .vl_right_margin = 64,
>>> + .vl_vsync_len = 2,
>>> + .vl_upper_margin = 22,
>>> + .vl_lower_margin = 21,
>>> + .mmio = ATMEL_BASE_LCDC,
>>> +};
>>> +
>>> +void lcd_enable(void)
>>> +{
>>> + at91_set_a_periph(AT91_PIO_PORTA, 29, 1); /* power up */
>>> +}
>>> +
>>> +void lcd_disable(void)
>>> +{
>>
>> The PA29 is handed to the LCD IP in at91_lcd_hw_init(), isn't there some
>> register to switch the power on and off? Why using the pullup for this
>> purpose here?
>
> I will check this, if not needed, I will remove them.
>
>>> + at91_set_a_periph(AT91_PIO_PORTA, 29, 0); /* power down */
>>> +}
>>> +
>>> +static void sama5d3xek_lcd_hw_init(void)
>>> +{
>>> + gd->fb_base = CONFIG_SAMA5D3_LCD_BASE;
>>> +
>>> + at91_lcd_hw_init();
>>> +}
>>> +
>>> +#ifdef CONFIG_LCD_INFO
>>> +#include <nand.h>
>>> +#include <version.h>
>>> +
>>> +void lcd_show_board_info(void)
>>
>> This function is really subject to move to a arch/arm/lib/at91 ... We
>> should think about that.
>
> I will consider that.
Well, but please in a future patch, we need to talk about that before.
<snip>
>>> diff --git a/include/configs/sama5d3xek.h b/include/configs/sama5d3xek.h
>>> new file mode 100644
>>> index 0000000..63616d0
>>> --- /dev/null
>>> +++ b/include/configs/sama5d3xek.h
>>> @@ -0,0 +1,268 @@
>>> +/*
>>> + * Configuation settings for the SAMA5D3xEK board.
>>> + *
>>> + * Copyright (C) 2012 - 2013 Atmel
>>
>> your email?
>
> OK, I will add it.
>
>>> + *
>>> + * based on at91sam9m10g45ek.h by:
>>> + * Stelian Pop <stelian at popies.net>
>>> + * Lead Tech Design <www.leadtechdesign.com>
>>> + *
>>> + * See file CREDITS for list of people who contributed to this
>>> + * project.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation; either version 2 of
>>> + * the License, or (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>> + */
>>> +
>>> +#ifndef __CONFIG_H
>>> +#define __CONFIG_H
>>> +
>>> +#include <asm/hardware.h>
>>> +
>>> +#define CONFIG_SYS_TEXT_BASE 0x26f00000
>>> +
>>> +/* ARM asynchronous clock */
>>> +#define CONFIG_SYS_AT91_SLOW_CLOCK 32768
>>> +#define CONFIG_SYS_AT91_MAIN_CLOCK 12000000 /* from 12 MHz
>>> crystal */
>>> +#define CONFIG_SYS_HZ 1000
>>> +
>>> +#define CONFIG_AT91FAMILY
>>> +#define CONFIG_ARCH_CPU_INIT
>>> +#undef CONFIG_USE_IRQ /* we don't need IRQ/FIQ stuff */
>>
>> As mentioned before, please do not undef what was never defined before.
>
> OK.
>
>>> +
>>> +#undef CONFIG_CMDLINE_TAG /* enable passing of ATAGs */
>>> +#undef CONFIG_SETUP_MEMORY_TAGS
>>> +#undef CONFIG_INITRD_TAG
>>> +#define CONFIG_SKIP_LOWLEVEL_INIT
>>> +#define CONFIG_BOARD_EARLY_INIT_F
>>> +#define CONFIG_DISPLAY_CPUINFO
>>> +
>>> +#define CONFIG_CMD_BOOTZ
>>> +#define CONFIG_OF_LIBFDT /* Device Tree support */
>>> +
>>> +/* general purpose I/O */
>>> +#define CONFIG_ATMEL_LEGACY /* required until (g)pio is fixed */
>>
>> grrr ... we really _should_ fix it some day.
>
> Yes, that's true.
> For sama5d3x, we won't depend on it. we can remove this configuration.
Oh, great, please remove it here then.
Best regards
Andreas Bießmann
More information about the U-Boot
mailing list