[U-Boot] [PATCH v2 1/3] serial: uartlite: Move driver to DM

Simon Glass sjg at chromium.org
Fri Dec 18 03:40:28 CET 2015 

Hi Michal,

On 17 December 2015 at 11:52, Michal Simek <michal.simek at xilinx.com> wrote:
> On 17.12.2015 16:27, Simon Glass wrote:
>> Hi Michal,
>>
>> On 17 December 2015 at 06:58, Michal Simek <michal.simek at xilinx.com> wrote:
>>> On 17.12.2015 14:37, Thomas Chou wrote:
>>>> Hi Michal,
>>>>
>>>> On 2015年12月17日 20:00, Michal Simek wrote:
>>>>> Enable SPL DM too.
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Remove unneeded headers
>>>>> - Use get_dev_addr instead of fdtdec_get_addr
>>>>> - Use platdata instead of private data
>>>>> - Add opb compatible string to be in sync with Linux
>>>>> - Add binding documentation
>>>>>
>>>>>   arch/microblaze/Kconfig                            |   1 +
>>>>>   configs/microblaze-generic_defconfig               |   2 +
>>>>>   .../serial/xilinx_uartlite.txt                     |  13 ++
>>>>>   doc/driver-model/serial-howto.txt                  |   1 -
>>>>>   drivers/serial/serial_xuartlite.c                  | 170
>>>>> ++++++++-------------
>>>>>   5 files changed, 78 insertions(+), 109 deletions(-)
>>>>>   create mode 100644 doc/device-tree-bindings/serial/xilinx_uartlite.txt
>>>>>
>>>>> diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
>>>>> index 604f6815af5b..30ea484f48aa 100644
>>>>> --- a/arch/microblaze/Kconfig
>>>>> +++ b/arch/microblaze/Kconfig
>>>>> @@ -13,6 +13,7 @@ config TARGET_MICROBLAZE_GENERIC
>>>>>       select SUPPORT_SPL
>>>>>       select OF_CONTROL
>>>>>       select DM
>>>>> +    select DM_SERIAL
>>>>>
>>>>>   endchoice
>>>>>
>>>>> diff --git a/configs/microblaze-generic_defconfig
>>>>> b/configs/microblaze-generic_defconfig
>>>>> index 54aa3ef3d26f..5df080b6a87c 100644
>>>>> --- a/configs/microblaze-generic_defconfig
>>>>> +++ b/configs/microblaze-generic_defconfig
>>>>> @@ -1,9 +1,11 @@
>>>>>   CONFIG_MICROBLAZE=y
>>>>>   CONFIG_SPL_SYS_MALLOC_SIMPLE=y
>>>>> +CONFIG_SPL_DM=y
>>>>>   CONFIG_TARGET_MICROBLAZE_GENERIC=y
>>>>>   CONFIG_DEFAULT_DEVICE_TREE="microblaze-generic"
>>>>>   CONFIG_SPL=y
>>>>>   CONFIG_SYS_PROMPT="U-Boot-mONStR> "
>>>>>   CONFIG_CMD_GPIO=y
>>>>>   # CONFIG_CMD_SETEXPR is not set
>>>>> +CONFIG_SPL_OF_CONTROL=y
>>>>>   CONFIG_OF_EMBED=y
>>>>> diff --git a/doc/device-tree-bindings/serial/xilinx_uartlite.txt
>>>>> b/doc/device-tree-bindings/serial/xilinx_uartlite.txt
>>>>> new file mode 100644
>>>>> index 000000000000..d15753c8c380
>>>>> --- /dev/null
>>>>> +++ b/doc/device-tree-bindings/serial/xilinx_uartlite.txt
>>>>> @@ -0,0 +1,13 @@
>>>>> +Binding for Xilinx Uartlite Controller
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : should be "xlnx,xps-uartlite-1.00.a", or
>>>>> "xlnx,opb-uartlite-1.00.b"
>>>>> +- reg: Should contain UART controller registers location and length.
>>>>> +- interrupts: Should contain UART controller interrupts.
>>>>> +
>>>>> +Example:
>>>>> +    serial at 40600000 {
>>>>> +        compatible = "xlnx,xps-uartlite-1.00.a";
>>>>> +        interrupts = <1 0>;
>>>>> +        reg = <0x40600000 0x10000>;
>>>>> +    };
>>>>> diff --git a/doc/driver-model/serial-howto.txt
>>>>> b/doc/driver-model/serial-howto.txt
>>>>> index 76ad629ef9cb..381a2a084562 100644
>>>>> --- a/doc/driver-model/serial-howto.txt
>>>>> +++ b/doc/driver-model/serial-howto.txt
>>>>> @@ -18,7 +18,6 @@ is time for maintainers to start converting over the
>>>>> remaining serial drivers:
>>>>>      serial_pxa.c
>>>>>      serial_s3c24x0.c
>>>>>      serial_sa1100.c
>>>>> -   serial_xuartlite.c
>>>>>      usbtty.c
>>>>>
>>>>>   You should complete this by the end of January 2016.
>>>>> diff --git a/drivers/serial/serial_xuartlite.c
>>>>> b/drivers/serial/serial_xuartlite.c
>>>>> index 988438e75471..8225d9a320a5 100644
>>>>> --- a/drivers/serial/serial_xuartlite.c
>>>>> +++ b/drivers/serial/serial_xuartlite.c
>>>>> @@ -1,5 +1,5 @@
>>>>>   /*
>>>>> - * (C) Copyright 2008-2011 Michal Simek <monstr at monstr.eu>
>>>>> + * (C) Copyright 2008 - 2015 Michal Simek <monstr at monstr.eu>
>>>>>    * Clean driver and add xilinx constant from header file
>>>>>    *
>>>>>    * (C) Copyright 2004 Atmark Techno, Inc.
>>>>> @@ -10,11 +10,15 @@
>>>>>
>>>>>   #include <config.h>
>>>>>   #include <common.h>
>>>>> +#include <dm.h>
>>>>>   #include <asm/io.h>
>>>>>   #include <linux/compiler.h>
>>>>>   #include <serial.h>
>>>>>
>>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>>> +
>>>>>   #define SR_TX_FIFO_FULL        0x08 /* transmit FIFO full */
>>>>> +#define SR_TX_FIFO_EMPTY    0x04 /* transmit FIFO empty */
>>>>>   #define SR_RX_FIFO_VALID_DATA    0x01 /* data in receive FIFO */
>>>>>   #define SR_RX_FIFO_FULL        0x02 /* receive FIFO full */
>>>>>
>>>>> @@ -28,135 +32,85 @@ struct uartlite {
>>>>>       unsigned int control;
>>>>>   };
>>>>>
>>>>> -static struct uartlite *userial_ports[4] = {
>>>>> -#ifdef XILINX_UARTLITE_BASEADDR
>>>>> -    [0] = (struct uartlite *)XILINX_UARTLITE_BASEADDR,
>>>>> -#endif
>>>>> -#ifdef XILINX_UARTLITE_BASEADDR1
>>>>> -    [1] = (struct uartlite *)XILINX_UARTLITE_BASEADDR1,
>>>>> -#endif
>>>>> -#ifdef XILINX_UARTLITE_BASEADDR2
>>>>> -    [2] = (struct uartlite *)XILINX_UARTLITE_BASEADDR2,
>>>>> -#endif
>>>>> -#ifdef XILINX_UARTLITE_BASEADDR3
>>>>> -    [3] = (struct uartlite *)XILINX_UARTLITE_BASEADDR3
>>>>> -#endif
>>>>> +struct uartlite_platdata {
>>>>> +    struct uartlite *regs;
>>>>>   };
>>>>>
>>>>> -static void uartlite_serial_putc(const char c, const int port)
>>>>> +static int uartlite_serial_putc(struct udevice *dev, const char ch)
>>>>>   {
>>>>> -    struct uartlite *regs = userial_ports[port];
>>>>> +    struct uartlite_platdata *plat = dev_get_platdata(dev);
>>>>> +    struct uartlite *regs = plat->regs;
>>>>>
>>>>> -    if (c == '\n')
>>>>> -        uartlite_serial_putc('\r', port);
>>>>> +    if (in_be32(&regs->status) & SR_TX_FIFO_FULL)
>>>>> +        return -EAGAIN;
>>>>>
>>>>> -    while (in_be32(&regs->status) & SR_TX_FIFO_FULL)
>>>>> -        ;
>>>>> -    out_be32(&regs->tx_fifo, c & 0xff);
>>>>> -}
>>>>> +    out_be32(&regs->tx_fifo, ch & 0xff);
>>>>>
>>>>> -static void uartlite_serial_puts(const char *s, const int port)
>>>>> -{
>>>>> -    while (*s)
>>>>> -        uartlite_serial_putc(*s++, port);
>>>>> +    return 0;
>>>>>   }
>>>>>
>>>>> -static int uartlite_serial_getc(const int port)
>>>>> +static int uartlite_serial_getc(struct udevice *dev)
>>>>>   {
>>>>> -    struct uartlite *regs = userial_ports[port];
>>>>> +    struct uartlite_platdata *plat = dev_get_platdata(dev);
>>>>> +    struct uartlite *regs = plat->regs;
>>>>> +
>>>>> +    if (!(in_be32(&regs->status) & SR_RX_FIFO_VALID_DATA))
>>>>> +        return -EAGAIN;
>>>>>
>>>>> -    while (!(in_be32(&regs->status) & SR_RX_FIFO_VALID_DATA))
>>>>> -        ;
>>>>>       return in_be32(&regs->rx_fifo) & 0xff;
>>>>>   }
>>>>>
>>>>> -static int uartlite_serial_tstc(const int port)
>>>>> +static int uartlite_serial_pending(struct udevice *dev, bool input)
>>>>>   {
>>>>> -    struct uartlite *regs = userial_ports[port];
>>>>> +    struct uartlite_platdata *plat = dev_get_platdata(dev);
>>>>> +    struct uartlite *regs = plat->regs;
>>>>>
>>>>> -    return in_be32(&regs->status) & SR_RX_FIFO_VALID_DATA;
>>>>> +    if (input)
>>>>> +        return in_be32(&regs->status) & SR_RX_FIFO_VALID_DATA;
>>>>> +
>>>>> +    return in_be32(&regs->status) & SR_TX_FIFO_EMPTY;
>>>>
>>>> Should be inversed.
>>>>
>>>> Otherwise,
>>>> Reviewed-by: Thomas Chou <thomas at wytron.com.tw>
>>>
>>> Can you please tell me more about it?
>>> I was trying to understand logic of this function but
>>> pending function is called just from here. (maybe I missed other
>>> occurrences but
>>> drivers/serial/serial-uclass.c:157:     if (ops->pending)
>>> drivers/serial/serial-uclass.c:158:             return ops->pending(dev,
>>> true);
>>>
>>> And not for output.
>>>
>>> From docs
>>> @return number of waiting characters, 0 for none, -ve on error
>>>
>>> It means I do read status and mask it if tx fifo is empty or not.
>>> If it is empty I am returning 0, if it is not empty I do return non 0 value.
>>>
>>> The similar logix is used for input.
>>>
>>> Is this logic wrong?
>>
>> Be careful to return 1 if you don't know how many characters are
>> waiting. It looks like you return 'in_be32(&regs->status) &
>> SR_RX_FIFO_VALID_DATA' which could be >1.
>
> IP doesn't provide information how many chars is in FIFO. It can be 1 to
> 16.
> No problem to return just 1 for more than 1 char in FIFO.
> Is core working with that information?

It has no effect right now, but in the future we might use it. If you
know there is at least one character, it is safe to return 1.

I suppose the UART does not support reading a single byte?

Regards,
Simon

More information about the U-Boot mailing list