From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Chou Date: Fri, 25 Dec 2015 14:48:14 +0800 Subject: [U-Boot] [PATCH v3 2/4] mips: ath79: add serial driver for ar933x SOC In-Reply-To: References: <1450956123-17606-1-git-send-email-wills.wang@live.com> <567CAC48.5060101@wytron.com.tw> Message-ID: <567CE6AE.6020506@wytron.com.tw> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 2015?12?25? 14:05, Wills Wang wrote: > > > On 12/25/2015 10:39 AM, Thomas Chou wrote: >> Hi Wills, >> >> Is there any reason that you need to support pre-DM serial driver? It >> should be safe to support DM only. You may add debug_uart support >> which might be helpful during debug. The ops for DM serial is >> different to pre-DM though the name look similar. Please see the >> comments below. >> > I will remove the code about pre-DM serial driver. >> I worked on mips before I moved to nios very long ago (20 yr). And >> nios2 mostly followed mips. >> >> I understand there is not device tree control for u-boot on mips yet. >> But I added it to nios2 recently. It might be worthy if you could add >> it to mips. You may find the dts binding on Linux kernel. And please >> add dts binding to doc/ and you might copy them from Linux kernel. >> > I has taken a long time to add base device tree for this patch, but i > can't get the driver serial number if don't specify explicitly "aliases" > section in dts file. but It seems to not need for ARM device. >>> + You will need to add "stdout-path=..." property with your serial path to the chosen node, like this, chosen { stdout-path = &uart; }; >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> Please sort the sequence of header files inclusion. >> >>> + >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> +struct ar933x_serial_baudrate{ >>> + u32 baudrate; >>> + u32 scale; >>> + u32 step; >>> +}; >>> + >>> +const struct ar933x_serial_baudrate baudrate_table_40mhz[] = { >>> +/* baudrate, scale, step */ >>> + {600, 255, 503}, >>> + {1200, 249, 983}, >>> + {2400, 167, 1321}, >>> + {4800, 87, 1384}, >>> + {9600, 45, 1447}, >>> + {14400, 53, 2548}, >>> + {19200, 22, 1447}, >>> + {28800, 26, 2548}, >>> + {38400, 28, 3649}, >>> + {56000, 7, 1468}, >>> + {57600, 34, 6606}, >>> + {115200, 28, 10947}, >>> + {128000, 6, 2936}, >>> + {153600, 18, 9563}, >>> + {230400, 16, 12834}, >>> + {250000, 4, 4096}, >>> + {256000, 6, 5872}, >>> + {460800, 7, 12079}, >>> + {576000, 4, 9437}, >>> + {921600, 3, 12079}, >>> + {1000000, 2, 9830}, >>> + {1152000, 2, 11324}, >>> + {1500000, 0, 4915}, >>> + {2000000, 0, 6553}, >>> + }; >>> + >>> +const struct ar933x_serial_baudrate baudrate_table_25mhz[] = { >>> +/* baudrate, scale, step */ >>> + {600, 255, 805}, >>> + {1200, 209, 1321}, >>> + {2400, 104, 1321}, >>> + {4800, 54, 1384}, >>> + {9600, 78, 3976}, >>> + {14400, 98, 7474}, >>> + {19200, 55, 5637}, >>> + {28800, 77, 11777}, >>> + {38400, 36, 7449}, >>> + {56000, 4, 1468}, >>> + {57600, 35, 10871}, >>> + {115200, 20, 12683}, >>> + {128000, 11, 8053}, >>> + {153600, 9, 8053}, >>> + {230400, 9, 12079}, >>> + {250000, 6, 9175}, >>> + {256000, 5, 8053}, >>> + {460800, 4, 12079}, >>> + {576000, 3, 12079}, >>> + {921600, 1, 9663}, >>> + {1000000, 1, 10485}, >>> + {1152000, 1, 12079}, >>> + {1500000, 0, 7864}, >>> + {2000000, 0, 10485}, >>> +}; >> >> The requirement of u-boot is much simpler than Linux kernel. For the >> uart header, you should include only the macros that the driver really >> used. Or you can add it to the driver directly without the additional >> header file. The same rule applies to the baudrate tables. You need >> only some practical rates, not every possible one. >> >>> + >>> +static inline u32 ar933x_read(u32 base, u32 offset) >>> +{ >>> + return readl(KSEG1ADDR(base + offset)); >>> +} >>> + >>> +static inline void ar933x_write(u32 base, u32 offset, u32 val) >>> +{ >>> + writel(val, KSEG1ADDR(base + offset)); >>> +} >>> + >> >> For the KSEG1ADDR mapping, which Marek also mentioned, I would suggest >> rework map_physmem() in asm/io.h on mips to map K1 kernel space, which >> is very similar to ioremap() on Linux kernel. So that you don't need >> to map it for every IO. >> >> plat->regs = map_physmem(dev_get_addr(dev), >> sizeof(...), >> MAP_NOCACHE); >> > In MIPS, function "map_physmem" is empty. Yes. But you may improve it to do the real work. For example, not tested, #define MAP_NOCACHE 1 #define MAP_WRCOMBINE 0 #define MAP_WRBACK 0 #define MAP_WRTHROUGH 0 static inline void * map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags) { if (flags) return (void *)KSEG1ADDR(paddr); else return (void *)KSEG0ADDR(paddr); } >>> +static int ar933x_serial_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 = ar933x_read(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_OE); >>> + val |= BIT(10); >>> + ar933x_write(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_OE, val); >>> + >>> + val = ar933x_read(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_FUNC); >>> + val |= (AR933X_GPIO_FUNC_UART_EN | BIT(15)); >>> + ar933x_write(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_FUNC, val); >> >> These might go to the pinctrl driver or board initialization. >> >>> + >>> + /* >>> + * UART controller configuration: >>> + * - no DMA >>> + * - no interrupt >>> + * - DCE mode >>> + * - no flow control >>> + * - set RX ready oride >>> + * - set TX ready oride >>> + */ >>> + val = AR933X_UART_CS_TX_READY_ORIDE | AR933X_UART_CS_RX_READY_ORIDE >>> + | (AR933X_UART_CS_IF_MODE_DCE << AR933X_UART_CS_IF_MODE_S); >>> + ar933x_write(AR933X_UART_BASE, AR933X_UART_CS_REG, val); >>> + return 0; >>> +} >>> + >>> +#ifdef CONFIG_DM_SERIAL >>> +static int ar933x_serial_setbrg(struct udevice *dev, int baudrate) >>> +{ >>> +#else >>> +static void ar933x_serial_setbrg(void) >>> +{ >>> + int baudrate = gd->baudrate; >>> +#endif >>> + u32 val, scale, step; >>> + const struct ar933x_serial_baudrate *baudrate_table; >>> + int i, baudrate_table_size; >>> + >>> + val = ar933x_read(AR71XX_RESET_BASE, AR933X_RESET_REG_BOOTSTRAP); >> >> This can go to some clock rate in board info during board initialization. >> >>> + if (val & AR933X_BOOTSTRAP_REF_CLK_40) { >>> + baudrate_table = baudrate_table_40mhz; >>> + baudrate_table_size = ARRAY_SIZE(baudrate_table_40mhz); >>> + scale = (40000000 / (16 * baudrate)) - 1; >>> + step = 8192; >>> + } else { >>> + baudrate_table = baudrate_table_25mhz; >>> + baudrate_table_size = ARRAY_SIZE(baudrate_table_25mhz); >>> + scale = (25000000 / (16 * baudrate)) - 1; >>> + step = 8192; >>> + } >>> + >>> + for (i = 0; i < baudrate_table_size; i++) { >>> + if (baudrate_table[i].baudrate == gd->baudrate) { >>> + scale = baudrate_table[i].scale; >>> + step = baudrate_table[i].step; >>> + } >>> + } >> >> What happens if not found? > "scale" and "step" was initialized by "if...else" statement above. >> >>> + >>> + val = ((scale & AR933X_UART_CLOCK_SCALE_M) >>> + << AR933X_UART_CLOCK_SCALE_S); >> >> The outer parenthesis is not needed. >> >>> + val |= ((step & AR933X_UART_CLOCK_STEP_M) >>> + << AR933X_UART_CLOCK_STEP_S); >>> + ar933x_write(AR933X_UART_BASE, AR933X_UART_CLOCK_REG, val); >>> +#ifdef CONFIG_DM_SERIAL >>> + return 0; >>> +#endif >>> +} >>> + >>> +#ifdef CONFIG_DM_SERIAL >>> +static int ar933x_serial_putc(struct udevice *dev, const char c) >>> +#else >>> +static void ar933x_serial_putc(const char c) >>> +#endif >>> +{ >>> + u32 data; >>> + >>> + if (c == '\n') >>> +#ifdef CONFIG_DM_SERIAL >>> + ar933x_serial_putc(dev, '\r'); >> >> Not needed for DM serial. >> >>> +#else >>> + ar933x_serial_putc('\r'); >>> +#endif >>> + do { >>> + data = ar933x_read(AR933X_UART_BASE, AR933X_UART_DATA_REG); >>> + } while (!(data & AR933X_UART_DATA_TX_CSR)); >> >> Return -EAGAIN if not ready for DM serial. >> >>> + >>> + data = (u32)c | AR933X_UART_DATA_TX_CSR; >>> + ar933x_write(AR933X_UART_BASE, AR933X_UART_DATA_REG, data); >>> +#ifdef CONFIG_DM_SERIAL >>> + return 0; >>> +#endif >>> +} >>> + >>> +#ifdef CONFIG_DM_SERIAL >>> +static int ar933x_serial_getc(struct udevice *dev) >>> +#else >>> +static int ar933x_serial_getc(void) >>> +#endif >>> +{ >>> + u32 data; >>> + >>> + do { >>> + data = ar933x_read(AR933X_UART_BASE, AR933X_UART_DATA_REG); >>> + } while (!(data & AR933X_UART_DATA_RX_CSR)); >> >> Return -EAGAIN if no data available for DM serial. >> >>> + >>> + data = ar933x_read(AR933X_UART_BASE, AR933X_UART_DATA_REG); >>> + ar933x_write(AR933X_UART_BASE, AR933X_UART_DATA_REG, >>> AR933X_UART_DATA_RX_CSR); >>> + return data & AR933X_UART_DATA_TX_RX_MASK; >>> +} >>> + >>> +#ifdef CONFIG_DM_SERIAL >>> +static int ar933x_serial_pending(struct udevice *dev, bool input) >>> +{ >>> + u32 data; >>> + >>> + data = ar933x_read(AR933X_UART_BASE, AR933X_UART_DATA_REG); >>> + if (input) >>> + return (data & AR933X_UART_DATA_RX_CSR) ? 1 : 0; >>> + else >>> + return (data & AR933X_UART_DATA_TX_CSR) ? 0 : 1; >>> +} >>> + >>> +static int ar933x_serial_probe(struct udevice *dev) >>> +{ >>> + ar933x_serial_init(); >>> + return 0; >>> +} >>> + >>> +static const struct dm_serial_ops ar933x_serial_ops = { >>> + .putc = ar933x_serial_putc, >>> + .pending = ar933x_serial_pending, >>> + .getc = ar933x_serial_getc, >>> + .setbrg = ar933x_serial_setbrg, >>> +}; >>> + >>> +static const struct udevice_id ar933x_serial_ids[] = { >>> + { .compatible = "ath79,ar933x-uart" }, >>> + { } >>> +}; >>> + >>> +U_BOOT_DRIVER(serial_ar933x) = { >>> + .name = "serial_ar933x", >>> + .id = UCLASS_SERIAL, >>> + .of_match = ar933x_serial_ids, >>> + .probe = ar933x_serial_probe, >>> + .ops = &ar933x_serial_ops, >>> + .flags = DM_FLAG_PRE_RELOC, >>> +}; >> >> I understand there might be only a single UART on the ar9331. But it >> might be better to bind the reg address with U_BOOT_DEVICE() and >> platform data, or better dts. >> > The pinctrl and clock drvier is not ready, there is no way to operate > clock and gpio resister if use address from device tree. You may add a legacy get_serial_clock() temporarily. And put the reset/pin configuration under mach-ath79. You may improve them with pinctrl and clock drivers. Best regards, Thomas