Re: [PATCH V4 1/2] xen/arm: Add i.MX lpuart driver

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 11.04.2022 12:20, Julien Grall wrote:
> Hi,
> 
> On 11/04/2022 07:26, Michal Orzel wrote:
>> On 08.04.2022 16:09, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@xxxxxxx>
>>>
>>> The i.MX LPUART Documentation:
>>> https://www.nxp.com/webapp/Download?colCode=IMX8QMIEC
>>> Chatper 13.6 Low Power Universal Asynchronous Receiver/
>>> Transmitter (LPUART)
>>>
>>> Tested-by: Henry Wang <Henry.Wang@xxxxxxx>
>>> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
>>> ---
>>>   xen/arch/arm/include/asm/imx-lpuart.h |  64 ++++++
>>>   xen/drivers/char/Kconfig              |   7 +
>>>   xen/drivers/char/Makefile             |   1 +
>>>   xen/drivers/char/imx-lpuart.c         | 276 ++++++++++++++++++++++++++
>>>   4 files changed, 348 insertions(+)
>>>   create mode 100644 xen/arch/arm/include/asm/imx-lpuart.h
>>>   create mode 100644 xen/drivers/char/imx-lpuart.c
>>>
>>> diff --git a/xen/arch/arm/include/asm/imx-lpuart.h 
>>> b/xen/arch/arm/include/asm/imx-lpuart.h
>>> new file mode 100644
>>> index 0000000000..26e2cf0249
>>> --- /dev/null
>>> +++ b/xen/arch/arm/include/asm/imx-lpuart.h
>>> @@ -0,0 +1,64 @@
>>> +/*
>>> + * xen/arch/arm/include/asm/imx-lpuart.h
>>> + *
>>> + * Common constant definition between early printk and the LPUART driver
>>> + *
>>> + * Peng Fan <peng.fan@xxxxxxx>
>>> + * Copyright 2022 NXP
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#ifndef __ASM_ARM_IMX_LPUART_H__
>>> +#define __ASM_ARM_IMX_LPUART_H__
>>> +
>>> +/* 32-bit register definition */
>>> +#define UARTBAUD          (0x10)
>>> +#define UARTSTAT          (0x14)
>>> +#define UARTCTRL          (0x18)
>>> +#define UARTDATA          (0x1C)
>>> +#define UARTMATCH         (0x20)
>>> +#define UARTMODIR         (0x24)
>>> +#define UARTFIFO          (0x28)
>>> +#define UARTWATER         (0x2c)
>>> +
>>> +#define UARTSTAT_TDRE     BIT(23, UL)
>>> +#define UARTSTAT_TC       BIT(22, UL)
>>> +#define UARTSTAT_RDRF     BIT(21, UL)
>>> +#define UARTSTAT_OR       BIT(19, UL)
>>> +
>>> +#define UARTBAUD_OSR_SHIFT (24)
>>> +#define UARTBAUD_OSR_MASK (0x1f)
>>> +#define UARTBAUD_SBR_MASK (0x1fff)
>>> +#define UARTBAUD_BOTHEDGE (0x00020000)
>>> +#define UARTBAUD_TDMAE    (0x00800000)
>>> +#define UARTBAUD_RDMAE    (0x00200000)It seems like you missed my comment 
>>> about alignment from the previous patch version.
>> Please keep the same alignment of values within the same section.
> 
> I haven't yet reviewed this patch. But if this is the only comment, then I 
> can update the alignment on commit.
> 
> Cheers,
> 

All my other findings have already been fixed by Peng. This is the last one as 
it was forgotten.
Patch 2/2 still has sth to be fixed so the alignment can be fixed in the same 
shot as well.

Cheers,
Michal