Re: [PATCH v1 tty] 8250: microchip: Add 4 Mbps support in PCI1XXXX UART

[Rengarajan.S]

Hi Andy Shevchenko,

Thanks for reviewing the patch. Please find my comments inline.

On Sun, 2024-01-28 at 17:27 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, Jan 25, 2024 at 03:36:19PM +0530, Rengarajan S wrote:
> > The current clock input is set to 62.5 MHz for supporting
> > fractional
> > divider, which enables generation of an acceptable baud rate from
> > any
> > frequency. With the current clock input the baud rate range is
> > limited
> > to 3.9 Mbps. Hence, the current range is extended to support 4 Mbps
> > with Burst mode operation. Divisor calculation for a given baud
> > rate is
> > updated as the sampling rate is reduced from 16 to 8 for 4 Mbps.
> 
> ...
> 
> > +#define UART_BAUD_4MBPS                              4000000
> 
> (4 * MEGA) ? (will need to include units.h, if not yet)

Thanks. Will address the change in the next patch revision.

> 
> ...
> 
> > +     frac_div = readl(port->membase + FRAC_DIV_CFG_REG);
> 
> > +
> 
> Unneeded blank line.

Will remove it in the next patch revision.

> 
> > +     if (frac_div == UART_BIT_DIVISOR_16)
> > +             sample_cnt = UART_BIT_SAMPLE_CNT_16;
> > +     else
> > +             sample_cnt = UART_BIT_SAMPLE_CNT_8;
> 
> ...
> 
> > +     /*
> > +      * Microchip PCI1XXXX UART supports maximum baud rate up to 4
> > Mbps
> > +      */
> > +     if (up->port.type == PORT_MCHP16550A)
> > +             max = 4000000;
> 
> No. Please refactor the way the 8250_port won't be modified.
> 
> Also you have a define for this constant, use it.


The current UART clk in MCHP Ports in pci1xxxx.c is set to 62.5 MHz in
order to support fractional baud rates which enables generation of
acceptable baud rate and lower error percentage from any available
frequency. With 62.5 MHz the maximum supported baud rate supported as
per serial_8250_get_baud_rate is 3.9 Mbps. In order to extend the
support to 4 Mbps we had hardcoded the max value to 4 Mbps. Since, baud
rate is calculated here we needed to make these changes in 8250_port
and could not find a way to handle as part 8250_pci1xxxx. Can you let
us know any alternatives to address this upper(max) limit? 

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>