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

  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@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci1xxxx.c | 25 ++++++++++++++++++-------
 drivers/tty/serial/8250/8250_port.c     |  6 ------
 2 files changed, 18 insertions(+), 13 deletions(-)
  

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@linux.intel.com>
> ---
>  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@microchip.com>
  

On Wed, Feb 14, 2024 at 03:50:09PM +0200, Andy Shevchenko wrote:
> 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@linux.intel.com>

Breaks the build:

drivers/tty/serial/8250/8250_port.c: In function ‘serial8250_get_baud_rate’:
drivers/tty/serial/8250/8250_port.c:2684:32: error: unused variable ‘up’ [-Werror=unused-variable]
 2684 |         struct uart_8250_port *up = up_to_u8250p(port);
      |                                ^~
cc1: all warnings being treated as errors
  

On Thu, Feb 15, 2024 at 09:26:21AM +0000, Rengarajan.S@microchip.com wrote:
> On Wed, 2024-02-14 at 15:50 +0200, Andy Shevchenko wrote:

..

> > +       /*
> > +        * 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? 

I specifically added the above comment. If you look closer, your driver does
not use this value at all, the 8250 port code uses it in several places:

- 8250_rsa case (not applicable to your driver)

- probe_baud() call (applicable iff the kernel command line misses the
  baudrate, but even without this patch it's broken for your driver)

- serial8250_update_uartclk() call (not applicable to your driver)

- serial8250_get_baud_rate() call (only to get max and min range;
  my change will have an effect on min (max is exactly what your
  quirk is doing right no), so 62500000/16/65535 ~= 59.6, while
  with my change 64000000/16/65535 ~= 61.0, but standard baudrate
  here is 50 and 75, the former isn't supported by the existing
  code either

- serial8250_do_get_divisor() call when magic_multiplier supplied
  (not applicable to your driver)

- autoconfig_16550a() call (not applicable to your driver)

Hope this clarifies the case.

Of course if you able to test, will be even better.
But wait for v2 where I update what Greg caught.

..

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

Thank you!
  

On Mon, 2024-02-19 at 18:19 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, Feb 15, 2024 at 09:26:21AM +0000,
> Rengarajan.S@microchip.com wrote:
> > On Wed, 2024-02-14 at 15:50 +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > +       /*
> > > +        * 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?
> 
> I specifically added the above comment. If you look closer, your
> driver does
> not use this value at all, the 8250 port code uses it in several
> places:
> 
> - 8250_rsa case (not applicable to your driver)
> 
> - probe_baud() call (applicable iff the kernel command line misses
> the
>   baudrate, but even without this patch it's broken for your driver)
> 
> - serial8250_update_uartclk() call (not applicable to your driver)
> 
> - serial8250_get_baud_rate() call (only to get max and min range;
>   my change will have an effect on min (max is exactly what your
>   quirk is doing right no), so 62500000/16/65535 ~= 59.6, while
>   with my change 64000000/16/65535 ~= 61.0, but standard baudrate
>   here is 50 and 75, the former isn't supported by the existing
>   code either
> 
> - serial8250_do_get_divisor() call when magic_multiplier supplied
>   (not applicable to your driver)
> 
> - autoconfig_16550a() call (not applicable to your driver)
> 
> Hope this clarifies the case.
> 
> Of course if you able to test, will be even better.
> But wait for v2 where I update what Greg caught.

Thanks for the clarification Andy. Will start with the testing after v2
patch.

> 
> ...
> 
> > Acked-by: Rengarajan S <rengarajan.s@microchip.com>
> 
> Thank you!
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>
  

On Tue, Feb 20, 2024 at 04:21:59AM +0000, Rengarajan.S@microchip.com wrote:
> On Mon, 2024-02-19 at 18:19 +0200, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 09:26:21AM +0000,
> > Rengarajan.S@microchip.com wrote:
> > > On Wed, 2024-02-14 at 15:50 +0200, Andy Shevchenko wrote:

..

> > > > +       /*
> > > > +        * 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?
> > 
> > I specifically added the above comment. If you look closer, your
> > driver does
> > not use this value at all, the 8250 port code uses it in several
> > places:
> > 
> > - 8250_rsa case (not applicable to your driver)
> > 
> > - probe_baud() call (applicable iff the kernel command line misses
> > the
> >   baudrate, but even without this patch it's broken for your driver)
> > 
> > - serial8250_update_uartclk() call (not applicable to your driver)
> > 
> > - serial8250_get_baud_rate() call (only to get max and min range;
> >   my change will have an effect on min (max is exactly what your
> >   quirk is doing right no), so 62500000/16/65535 ~= 59.6, while
> >   with my change 64000000/16/65535 ~= 61.0, but standard baudrate
> >   here is 50 and 75, the former isn't supported by the existing
> >   code either
> > 
> > - serial8250_do_get_divisor() call when magic_multiplier supplied
> >   (not applicable to your driver)
> > 
> > - autoconfig_16550a() call (not applicable to your driver)
> > 
> > Hope this clarifies the case.
> > 
> > Of course if you able to test, will be even better.
> > But wait for v2 where I update what Greg caught.
> 
> Thanks for the clarification Andy. Will start with the testing after v2
> patch.

v2 is here:

https://lore.kernel.org/r/20240219162917.2159736-1-andriy.shevchenko@linux.intel.com