[U-Boot] [PATCH 4/4] ARM: atmel: add sama5d3xek support

[Bo Shen]

Hi Andreas,

On 3/8/2013 15:32, Andreas Bießmann wrote:
[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.

Now, we depends on reset state. This the reset value of this register is 
0x00000000, so no protect.

If we really need the write protect feature, will add it in future.

[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?

It's my fault.
OK, I will put TX line unconditional pull up. And Rx line without pull up.

[snip]
>>>> +#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?

I will set them pull up unconditionally. The atmel spi driver will set 
the GPIO to 1 when activate, set to 0 when deactivate.

>
> <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.

I suggest we keep the lower 16 bit line here, and put the higher 8 bit 
line to board file (sama5d3xek.c). Then if someone want to use higher 8 
bit line which is different with sama5d3xek, then they can implement it 
in board file.

>>>> +    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

Best Regards,
Bo Shen