[U-Boot] [PATCH v5 4/8] x86: slimbootloader: Add serial driver

[Park, Aiden]

Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
> Sent: Monday, July 22, 2019 8:38 AM
> To: Park, Aiden <aiden.park at intel.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Simon Glass
> <sjg at chromium.org>; Bin Meng <bmeng.cn at gmail.com>
> Subject: Re: [PATCH v5 4/8] x86: slimbootloader: Add serial driver
> 
> On Wed, Jul 17, 2019 at 7:41 AM Park, Aiden <aiden.park at intel.com> wrote:
> >
> > Slim Bootloader provides serial port info thru its HOB list pointer.
> > All these HOBs are eligible for Slim Bootloader based board only.
> > - Get serial port information from the serial port info hob
> > - Leverage ns16550 driver with slimbootloader specific platform data
> 
> > +       /*
> > +        * The serial_info->type provides port io or mmio access type info,
> > +        * but the access type will be controlled by
> > +        * CONFIG_SYS_NS16550_PORT_MAPPED or
> CONFIG_SYS_NS16550_MEM32.
> > +        *
> > +        * TBD: ns16550 access type configuration in runtime.
> > +        *      ex) plat->access_type = serial_info->type
> > +        */
> 
> io -> IO
> mmio -> MMIO
> 
Let me change this.

> > +       plat->base = serial_info->base;
> > +       /* ns16550 uses reg_shift, then covert stride to shift */
> 
> > +       plat->reg_shift = (serial_info->stride >> 1);
> 
> Too many parenthesis.
>
Let me remove parenthesis.
 
> > +       plat->clock = serial_info->clk;
> > +
> > +       return 0;
> > +}
> 
> > +/**
> > + * A GUID to get SerialPort info hob which is provided by Slim
> > +Bootloader  */ #define LOADER_SERIAL_PORT_INFO_GUID \
> > +       { \
> > +       0x6c6872fe, 0x56a9, 0x4403, \
> > +       { 0xbb, 0x98, 0x95, 0x8d, 0x62, 0xde, 0x87, 0xf1 } \
> > +       }
> 
> EFI_GUID()
>
Let me use EFI_GUID.

> > +struct serial_port_info {
> 
> serial is not this platform namespace, choose more particular one, please.
> 
This header file is only for arch-slimbootloader. Anyway, let me use particular one.

> > +       u8      rev;
> > +       u8      rsvd[3];
> > +       u32     type;
> > +       u32     base;
> > +       u32     baud;
> > +       u32     stride;
> > +       u32     clk;
> > +       u32     rsvd1;
> 
> > +} __packed;
> 
> Does __packed service for anything here?
> 
Thanks. This is not necessary. Let me remove __packed.

> --
> With Best Regards,
> Andy Shevchenko

Best Regards,
Aiden