Re: [PATCH v1 1/1] serial: 8250_pci1xxxx: Drop quirk from 8250_port

[Rengarajan.S]

Hi Andy,

On Wed, 2024-02-14 at 15:50 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> We are not supposed to spread quirks in 8250_port module especially
> when we have a separate driver for the hardware in question.
> 
> Move quirk from generic module to the driver that uses it.
> 
> While at it, move IO to ->set_divisor() callback as it has to be from
> day 1. ->get_divisor() is not supposed to perform any IO as UART
> port:
> - might not be powered on
> - is not locked by a spin lock
> 
> Fixes: 1ed67ecd1349 ("8250: microchip: Add 4 Mbps support in PCI1XXXX
> UART")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 25 ++++++++++++++++++-----
> --
>  drivers/tty/serial/8250/8250_port.c     |  6 ------
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 55eada1dba56..2fbb5851f788 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -94,7 +94,6 @@
>  #define UART_BIT_SAMPLE_CNT_16                 16
>  #define BAUD_CLOCK_DIV_INT_MSK                 GENMASK(31, 8)
>  #define ADCL_CFG_RTS_DELAY_MASK                        GENMASK(11,
> 8)
> -#define UART_CLOCK_DEFAULT                     (62500 * HZ_PER_KHZ)
> 
>  #define UART_WAKE_REG                          0x8C
>  #define UART_WAKE_MASK_REG                     0x90
> @@ -227,13 +226,10 @@ static unsigned int pci1xxxx_get_divisor(struct
> uart_port *port,
>         unsigned int uart_sample_cnt;
>         unsigned int quot;
> 
> -       if (baud >= UART_BAUD_4MBPS) {
> +       if (baud >= UART_BAUD_4MBPS)
>                 uart_sample_cnt = UART_BIT_SAMPLE_CNT_8;
> -               writel(UART_BIT_DIVISOR_8, (port->membase +
> FRAC_DIV_CFG_REG));
> -       } else {
> +       else
>                 uart_sample_cnt = UART_BIT_SAMPLE_CNT_16;
> -               writel(UART_BIT_DIVISOR_16, (port->membase +
> FRAC_DIV_CFG_REG));
> -       }
> 
>         /*
>          * Calculate baud rate sampling period in nanoseconds.
> @@ -249,6 +245,11 @@ static unsigned int pci1xxxx_get_divisor(struct
> uart_port *port,
>  static void pci1xxxx_set_divisor(struct uart_port *port, unsigned
> int baud,
>                                  unsigned int quot, unsigned int
> frac)
>  {
> +       if (baud >= UART_BAUD_4MBPS)
> +               writel(UART_BIT_DIVISOR_8, port->membase +
> FRAC_DIV_CFG_REG);
> +       else
> +               writel(UART_BIT_DIVISOR_16, port->membase +
> FRAC_DIV_CFG_REG);
> +
>         writel(FIELD_PREP(BAUD_CLOCK_DIV_INT_MSK, quot) | frac,
>                port->membase + UART_BAUD_CLK_DIVISOR_REG);
>  }
> @@ -619,6 +620,17 @@ static int pci1xxxx_setup(struct pci_dev *pdev,
> 
>         port->port.flags |= UPF_FIXED_TYPE | UPF_SKIP_TEST;
>         port->port.type = PORT_MCHP16550A;
> +       /*
> +        * 8250 core considers prescaller value to be always 16.
> +        * The MCHP ports support downscaled mode and hence the
> +        * functional UART clock can be lower, i.e. 62.5MHz, than
> +        * software expects in order to support higher baud rates.
> +        * Assign here 64MHz to support 4Mbps.
> +        *
> +        * The value itself is not really used anywhere except baud
> +        * rate calculations, so we can mangle it as we wish.
> +        */
> +       port->port.uartclk = 64 * HZ_PER_MHZ;

As per internal MCHP DOS, PCI1XXXX driver uses a simple method of
converting "legacy 16 bit baud rate generator" to a "32 bit fractional
baud rate generator" which enables generation of an acceptable baud
rate from any valuable frequency.

This is applicable only when the baud clock selected is 62.5 MHz, so
when we configure the baud clock to 64 MHz(as above) will it be
downscaled to 62.5 MHz, thus supporting the above feature? 

Other changes looks good to me.

>         port->port.set_termios = serial8250_do_set_termios;
>         port->port.get_divisor = pci1xxxx_get_divisor;
>         port->port.set_divisor = pci1xxxx_set_divisor;
> @@ -732,7 +744,6 @@ static int pci1xxxx_serial_probe(struct pci_dev
> *pdev,
> 
>         memset(&uart, 0, sizeof(uart));
>         uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT;
> -       uart.port.uartclk = UART_CLOCK_DEFAULT;
>         uart.port.dev = dev;
> 
>         if (num_vectors == max_vec_reqd)
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index c37905ea3cae..d59dc219c899 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2699,12 +2699,6 @@ static unsigned int
> serial8250_get_baud_rate(struct uart_port *port,
>                 max = (port->uartclk + tolerance) / 16;
>         }
> 
> -       /*
> -        * Microchip PCI1XXXX UART supports maximum baud rate up to 4
> Mbps
> -        */
> -       if (up->port.type == PORT_MCHP16550A)
> -               max = 4000000;
> -
>         /*
>          * Ask the core to calculate the divisor for us.
>          * Allow 1% tolerance at the upper limit so uart clks
> marginally
> --
> 2.43.0.rc1.1.gbec44491f096
> 


Acked-by: Rengarajan S <rengarajan.s@xxxxxxxxxxxxx>