Hi Andrey,
On 20.07.2016 17:03, Andrey Smirnov wrote:
> On Wed, Jul 20, 2016 at 7:17 AM, Wadim Egorov <w.egorov@xxxxxxxxx> wrote:
>> Signed-off-by: Wadim Egorov <w.egorov@xxxxxxxxx>
>> ---
>> arch/arm/mach-rockchip/include/mach/debug_ll.h | 72 +++++++++++++++-----------
>> common/Kconfig | 6 +--
>> 2 files changed, 45 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/arm/mach-rockchip/include/mach/debug_ll.h b/arch/arm/mach-rockchip/include/mach/debug_ll.h
>> index c666b99..144cada 100644
>> --- a/arch/arm/mach-rockchip/include/mach/debug_ll.h
>> +++ b/arch/arm/mach-rockchip/include/mach/debug_ll.h
>> @@ -1,25 +1,31 @@
>> #ifndef __MACH_DEBUG_LL_H__
>> #define __MACH_DEBUG_LL_H__
>>
>> +#include <common.h>
>> #include <io.h>
>> +#include <mach/rk3188-regs.h>
>> +#include <mach/rk3288-regs.h>
>> +
>> +#ifdef CONFIG_ARCH_RK3188
>> +
>> +#define UART_CLOCK 100000000
>> +#define RK_DEBUG_SOC RK3188
>> +#define serial_out(a, v) writeb(v, a)
>> +#define serial_in(a) readb(a)
>> +
>> +#elif defined CONFIG_ARCH_RK3288
>> +
>> +#define UART_CLOCK 24000000
>> +#define RK_DEBUG_SOC RK3288
>> +#define serial_out(a, v) writel(v, a)
>> +#define serial_in(a) readl(a)
> These "serial_in/out" macros seem a bit redundant to me. What's the
> story behind them, why were they added?
writeb() does not work with RK3288. So I added serial_in/out macros to
split the different types of memory access to the uart registers.
>
>> -#if CONFIG_DEBUG_ROCKCHIP_UART_PORT == 0
>> -#define UART_BASE 0x10124000
>> -#endif
>> -#if CONFIG_DEBUG_ROCKCHIP_UART_PORT == 1
>> -#define UART_BASE 0x10126000
>> -#endif
>> -#if CONFIG_DEBUG_ROCKCHIP_UART_PORT == 2
>> -#define UART_BASE 0x20064000
>> -#endif
>> -#if CONFIG_DEBUG_ROCKCHIP_UART_PORT == 3
>> -#define UART_BASE 0x20068000
>> #endif
>>
>> -#define LSR_THRE 0x20 /* Xmit holding register empty */
>> -#define LSR (5 << 2)
>> -#define THR (0 << 2)
>> +#define __RK_UART_BASE(soc, num) soc##_UART##num##_BASE
>> +#define RK_UART_BASE(soc, num) __RK_UART_BASE(soc, num)
>>
>> +#define LSR_THRE 0x20 /* Xmit holding register empty */
>> #define LCR_BKSE 0x80 /* Bank select enable */
>> #define LSR (5 << 2)
>> #define THR (0 << 2)
>> @@ -33,28 +39,34 @@
>>
>> static inline void INIT_LL(void)
>> {
>> - unsigned int clk = 100000000;
>> - unsigned int divisor = clk / 16 / 115200;
>> -
>> - writeb(0x00, UART_BASE + LCR);
>> - writeb(0x00, UART_BASE + IER);
>> - writeb(0x07, UART_BASE + MDR);
>> - writeb(LCR_BKSE, UART_BASE + LCR);
>> - writeb(divisor & 0xff, UART_BASE + DLL);
>> - writeb(divisor >> 8, UART_BASE + DLM);
>> - writeb(0x03, UART_BASE + LCR);
>> - writeb(0x03, UART_BASE + MCR);
>> - writeb(0x07, UART_BASE + FCR);
>> - writeb(0x00, UART_BASE + MDR);
>> + void __iomem *base = (void *)RK_UART_BASE(RK_DEBUG_SOC,
>> + CONFIG_DEBUG_ROCKCHIP_UART_PORT);
> There's a IOMEM macro that you could use to avoid explicit casting.
ok
>
>> + unsigned int divisor = DIV_ROUND_CLOSEST(UART_CLOCK, 16 * 115200);
> I'd suggest CONFIG_BAUDRATE instead of hard-coded value.
ok
>
>> +
>> + serial_out(base + LCR, 0x00);
>> + serial_out(base + IER, 0x00);
>> + serial_out(base + MDR, 0x07);
>> + serial_out(base + LCR, LCR_BKSE);
>> + serial_out(base + DLL, divisor & 0xff);
>> + serial_out(base + DLM, divisor >> 8);
>> + serial_out(base + LCR, 0x03);
>> + serial_out(base + MCR, 0x03);
>> + serial_out(base + FCR, 0x07);
>> + serial_out(base + MDR, 0x00);
>> }
>>
>> static inline void PUTC_LL(char c)
>> {
>> + void __iomem *base = (void *)RK_UART_BASE(RK_DEBUG_SOC,
>> + CONFIG_DEBUG_ROCKCHIP_UART_PORT);
> IOMEM here as well.
ok
>
>> +
>> /* Wait until there is space in the FIFO */
>> - while ((readb(UART_BASE + LSR) & LSR_THRE) == 0);
>> + while ((serial_in(base + LSR) & LSR_THRE) == 0)
>> + ;
> You could probably separate this busy loop into a small inline
> function and re-use it below and in the code of the full-fledged
> driver.
I don't really see the point here.
Regards,
Wadim
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
