[U-Boot] [PATCH v3 2/4] mips: ath79: add serial driver for ar933x SOC
Thomas Chou
thomas at wytron.com.tw
Fri Dec 25 07:48:14 CET 2015
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 <common.h>
>>> +#include <serial.h>
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <asm/io.h>
>>> +#include <asm/addrspace.h>
>>> +#include <asm/types.h>
>>> +#include <asm/arch/ar71xx_regs.h>
>>> +#include <asm/arch/ar933x_uart.h>
>>
>> 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
More information about the U-Boot
mailing list