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

Thomas Chou thomas at wytron.com.tw
Fri Dec 18 09:12:47 CET 2015 

Hi Michal,

On 2015年12月18日 15:52, Michal Simek wrote:
> On 18.12.2015 00:35, Thomas Chou wrote:
>> Hi Michal,
>>
>> On 2015年12月17日 21:58, Michal Simek 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?
>>>
>>
>> +    return in_be32(&regs->status) & SR_TX_FIFO_EMPTY;
>>
>> Yes, the logic is wrong. Your code return non 0 if empty, and 0 if not
>> empty.
>
> right.
>
> return !in_be32(&regs->status) & SR_TX_FIFO_EMPTY;

As suggested by Simon, it might be better to return 1 when we don't know 
the count of pending.

if (input)
         return (in_be32(&regs->status) & SR_RX_FIFO_VALID_DATA) ? 1 : 0;

return (in_be32(&regs->status) & SR_TX_FIFO_EMPTY) ? 0 : 1;


>
>
>>
>> BTW, you might want to convert the following macros to BIT() with a
>> follow-up.
>>
>>>>>     #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 */
>
> NP with that.

Best regards,
Thomas

More information about the U-Boot mailing list