| Message ID | 20180202042709.21627-1-joel@jms.id.au |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [linux,dev-4.13] serial: 8250: Add Nuvoton NPCM UART | expand |
Thanks Joel! On 2 February 2018 at 06:27, Joel Stanley <joel@jms.id.au> wrote: > From: Tomer Maimon <tmaimon77@gmail.com> > > The Nuvoton UART is almost compatible with the 8250 driver when probed > via the 8250_of driver, however it requires some extra configuration > at startup. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > Documentation/devicetree/bindings/serial/8250.txt | 1 + > drivers/tty/serial/8250/8250_of.c | 1 + > drivers/tty/serial/8250/8250_port.c | 30 +++++++++++++++++++++++ > include/uapi/linux/serial_core.h | 3 +++ > include/uapi/linux/serial_reg.h | 5 ++++ > 5 files changed, 40 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt > index 419ff6c0a47f..e8d921bae665 100644 > --- a/Documentation/devicetree/bindings/serial/8250.txt > +++ b/Documentation/devicetree/bindings/serial/8250.txt > @@ -22,6 +22,7 @@ Required properties: > - "ti,da830-uart" > - "aspeed,ast2400-vuart" > - "aspeed,ast2500-vuart" > + - "nuvoton,npcm750-uart" > - "serial" if the port type is unknown. > - reg : offset and length of the register set for the device. > - interrupts : should contain uart interrupt. > diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c > index 0cf95fddccfc..a947e1e36930 100644 > --- a/drivers/tty/serial/8250/8250_of.c > +++ b/drivers/tty/serial/8250/8250_of.c > @@ -298,6 +298,7 @@ static const struct of_device_id of_platform_serial_table[] = { > { .compatible = "mrvl,mmp-uart", > .data = (void *)PORT_XSCALE, }, > { .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, }, > + { .compatible = "nuvoton,npcm750-uart", .data = (void *)PORT_NPCM, }, > { /* end of list */ }, > }; > MODULE_DEVICE_TABLE(of, of_platform_serial_table); > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index a5fe0e66c607..df52f9ddbb99 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -289,6 +289,16 @@ static const struct serial8250_config uart_config[] = { > .rxtrig_bytes = {1, 4, 8, 14}, > .flags = UART_CAP_FIFO | UART_CAP_AFE, > }, > + [PORT_NPCM] = { > + .name = "Nuvoton 16550", > + .fifo_size = 16, > + .tx_loadsz = 16, > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > + UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT, > + .rxtrig_bytes = {1, 4, 8, 14}, > + .flags = UART_CAP_FIFO, > + > + }, > }; > > /* Uart divisor latch read */ > @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) > UART_DA830_PWREMU_MGMT_FREE); > } > > + if (port->type == PORT_NPCM) { > + /* > + * Nuvoton calls the scratch register 'UART_TOR' (timeout > + * register). Enable it, and set TIOC (timeout interrupt > + * comparator) to be 0x20 for correct operation. > + */ > + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); > + } Can we set the TOR register dynamically (adding special IOCTL)? > + > #ifdef CONFIG_SERIAL_8250_RSA > /* > * If this is an RSA port, see if we can kick it up to the > @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, > return quot_16 >> 4; > } > > +/* Nuvoton NPCM UARTs have a custom divisor calculation */ > +static unsigned int npcm_get_divisor(struct uart_8250_port *up, > + unsigned int baud) > +{ > + struct uart_port *port = &up->port; > + > + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; > +} > + > static unsigned int serial8250_get_divisor(struct uart_8250_port *up, > unsigned int baud, > unsigned int *frac) > @@ -2485,6 +2513,8 @@ static unsigned int serial8250_get_divisor(struct uart_8250_port *up, > quot = 0x8002; > else if (up->port.type == PORT_XR17V35X) > quot = xr17v35x_get_divisor(up, baud, frac); > + else if (up->port.type == PORT_NPCM) > + quot = npcm_get_divisor(up, baud); > else > quot = uart_get_divisor(port, baud); > > diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h > index c34a2a3eeff5..4807dca14ea2 100644 > --- a/include/uapi/linux/serial_core.h > +++ b/include/uapi/linux/serial_core.h > @@ -271,4 +271,7 @@ > /* MPS2 UART */ > #define PORT_MPS2UART 116 > > +/* Nuvton UART */ > +#define PORT_NPCM 117 > + > #endif /* _UAPILINUX_SERIAL_CORE_H */ > diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h > index 5db76880b4ad..50fc96431eb5 100644 > --- a/include/uapi/linux/serial_reg.h > +++ b/include/uapi/linux/serial_reg.h > @@ -374,5 +374,10 @@ > #define UART_ALTR_EN_TXFIFO_LW 0x01 /* Enable the TX FIFO Low Watermark */ > #define UART_ALTR_TX_LOW 0x41 /* Tx FIFO Low Watermark */ > > + > +/* Nuvoton NPCM timeout register */ > +#define UART_NPCM_TOR 7 > +#define UART_NPCM_TOIE BIT(7) /* Timeout Interrupt Enable */ > + > #endif /* _LINUX_SERIAL_REG_H */ > > -- > 2.15.1 > cheers, Tomer
On Mon, Feb 5, 2018 at 7:39 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: > On 2 February 2018 at 06:27, Joel Stanley <joel@jms.id.au> wrote: >> /* Uart divisor latch read */ >> @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) >> UART_DA830_PWREMU_MGMT_FREE); >> } >> >> + if (port->type == PORT_NPCM) { >> + /* >> + * Nuvoton calls the scratch register 'UART_TOR' (timeout >> + * register). Enable it, and set TIOC (timeout interrupt >> + * comparator) to be 0x20 for correct operation. >> + */ >> + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); >> + } > > Can we set the TOR register dynamically (adding special IOCTL)? We would have to have a good reason to do so. Under what circumstances would a user need to change this dynamically? > >> + >> #ifdef CONFIG_SERIAL_8250_RSA >> /* >> * If this is an RSA port, see if we can kick it up to the >> @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, >> return quot_16 >> 4; >> } >> >> +/* Nuvoton NPCM UARTs have a custom divisor calculation */ >> +static unsigned int npcm_get_divisor(struct uart_8250_port *up, >> + unsigned int baud) >> +{ >> + struct uart_port *port = &up->port; >> + >> + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; >> +} Did you test this version of the driver on your hardware? I think I understand why you had the extra code, but does it affect operation in any of your tests? From what I understand the baud rates are often calculated to be "near enough". Cheers, Joel
On Tue, Feb 6, 2018 at 7:50 AM, Joel Stanley <joel@jms.id.au> wrote: > On Mon, Feb 5, 2018 at 7:39 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: >> On 2 February 2018 at 06:27, Joel Stanley <joel@jms.id.au> wrote: > >>> /* Uart divisor latch read */ >>> @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) >>> UART_DA830_PWREMU_MGMT_FREE); >>> } >>> >>> + if (port->type == PORT_NPCM) { >>> + /* >>> + * Nuvoton calls the scratch register 'UART_TOR' (timeout >>> + * register). Enable it, and set TIOC (timeout interrupt >>> + * comparator) to be 0x20 for correct operation. >>> + */ >>> + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); >>> + } >> >> Can we set the TOR register dynamically (adding special IOCTL)? > > We would have to have a good reason to do so. > > Under what circumstances would a user need to change this dynamically? Tomer, I agree it is not important to change at run time. > >> >>> + >>> #ifdef CONFIG_SERIAL_8250_RSA >>> /* >>> * If this is an RSA port, see if we can kick it up to the >>> @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, >>> return quot_16 >> 4; >>> } >>> >>> +/* Nuvoton NPCM UARTs have a custom divisor calculation */ >>> +static unsigned int npcm_get_divisor(struct uart_8250_port *up, >>> + unsigned int baud) >>> +{ >>> + struct uart_port *port = &up->port; >>> + >>> + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; >>> +} > > Did you test this version of the driver on your hardware? > > I think I understand why you had the extra code, but does it affect > operation in any of your tests? > > From what I understand the baud rates are often calculated to be "near enough". Joel, I made the original calculation but I agree that this one is good enough (I confused Tomer to think that your calculation will not fit :) > > Cheers, > > Joel
On 6 February 2018 at 12:37, Avi Fishman <avifishman70@gmail.com> wrote: > On Tue, Feb 6, 2018 at 7:50 AM, Joel Stanley <joel@jms.id.au> wrote: >> On Mon, Feb 5, 2018 at 7:39 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: >>> On 2 February 2018 at 06:27, Joel Stanley <joel@jms.id.au> wrote: >> >>>> /* Uart divisor latch read */ >>>> @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) >>>> UART_DA830_PWREMU_MGMT_FREE); >>>> } >>>> >>>> + if (port->type == PORT_NPCM) { >>>> + /* >>>> + * Nuvoton calls the scratch register 'UART_TOR' (timeout >>>> + * register). Enable it, and set TIOC (timeout interrupt >>>> + * comparator) to be 0x20 for correct operation. >>>> + */ >>>> + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); >>>> + } >>> >>> Can we set the TOR register dynamically (adding special IOCTL)? >> >> We would have to have a good reason to do so. >> >> Under what circumstances would a user need to change this dynamically? > > Tomer, I agree it is not important to change at run time. I agree Avi, but the only problem I see with it that if we will need in the future to TOR register dynamically (new SOC, client demand, etc.) Could it be done in the 8250 files? > >> >>> >>>> + >>>> #ifdef CONFIG_SERIAL_8250_RSA >>>> /* >>>> * If this is an RSA port, see if we can kick it up to the >>>> @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, >>>> return quot_16 >> 4; >>>> } >>>> >>>> +/* Nuvoton NPCM UARTs have a custom divisor calculation */ >>>> +static unsigned int npcm_get_divisor(struct uart_8250_port *up, >>>> + unsigned int baud) >>>> +{ >>>> + struct uart_port *port = &up->port; >>>> + >>>> + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; >>>> +} >> >> Did you test this version of the driver on your hardware? Yes Thanks!!! it working fine. >> >> I think I understand why you had the extra code, but does it affect >> operation in any of your tests? >> >> From what I understand the baud rates are often calculated to be "near enough". > > Joel, I made the original calculation but I agree that this one is > good enough (I confused Tomer to think that your calculation will not > fit :) > I see you add the TOR to include/uapi/linux/serial_reg.h will the maintainer accept it? >> >> Cheers, >> >> Joel Cheers, Tomer
On Tue, Feb 6, 2018 at 10:54 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: > On 6 February 2018 at 12:37, Avi Fishman <avifishman70@gmail.com> wrote: >> On Tue, Feb 6, 2018 at 7:50 AM, Joel Stanley <joel@jms.id.au> wrote: >>> On Mon, Feb 5, 2018 at 7:39 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: >>>> On 2 February 2018 at 06:27, Joel Stanley <joel@jms.id.au> wrote: >>> >>>>> /* Uart divisor latch read */ >>>>> @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) >>>>> UART_DA830_PWREMU_MGMT_FREE); >>>>> } >>>>> >>>>> + if (port->type == PORT_NPCM) { >>>>> + /* >>>>> + * Nuvoton calls the scratch register 'UART_TOR' (timeout >>>>> + * register). Enable it, and set TIOC (timeout interrupt >>>>> + * comparator) to be 0x20 for correct operation. >>>>> + */ >>>>> + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); >>>>> + } >>>> >>>> Can we set the TOR register dynamically (adding special IOCTL)? >>> >>> We would have to have a good reason to do so. >>> >>> Under what circumstances would a user need to change this dynamically? >> >> Tomer, I agree it is not important to change at run time. > I agree Avi, > but the only problem I see with it that if we will need in the future > to TOR register dynamically (new SOC, client demand, etc.) > Could it be done in the 8250 files? If we have a requirement in the future I suggest we decide how to solve that problem then. Cross that bridge when we come to it. >> >>> >>>> >>>>> + >>>>> #ifdef CONFIG_SERIAL_8250_RSA >>>>> /* >>>>> * If this is an RSA port, see if we can kick it up to the >>>>> @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, >>>>> return quot_16 >> 4; >>>>> } >>>>> >>>>> +/* Nuvoton NPCM UARTs have a custom divisor calculation */ >>>>> +static unsigned int npcm_get_divisor(struct uart_8250_port *up, >>>>> + unsigned int baud) >>>>> +{ >>>>> + struct uart_port *port = &up->port; >>>>> + >>>>> + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; >>>>> +} >>> >>> Did you test this version of the driver on your hardware? > Yes Thanks!!! it working fine. Great! >>> >>> I think I understand why you had the extra code, but does it affect >>> operation in any of your tests? >>> >>> From what I understand the baud rates are often calculated to be "near enough". >> >> Joel, I made the original calculation but I agree that this one is >> good enough (I confused Tomer to think that your calculation will not >> fit :) >> > I see you add the TOR to include/uapi/linux/serial_reg.h will the > maintainer accept it? It's in a private include, include/uapi/linux/serial_reg.h. Initially I didn't do this, but if you examine the end of that file there are a number of other 8250 ports that do this. If you would like, I will send this patch upstream now. Cheers, Joel
On 7 February 2018 at 03:50, Joel Stanley <joel@jms.id.au> wrote: > On Tue, Feb 6, 2018 at 10:54 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: >> On 6 February 2018 at 12:37, Avi Fishman <avifishman70@gmail.com> wrote: >>> On Tue, Feb 6, 2018 at 7:50 AM, Joel Stanley <joel@jms.id.au> wrote: >>>> On Mon, Feb 5, 2018 at 7:39 PM, Tomer Maimon <tmaimon77@gmail.com> wrote: >>>>> On 2 February 2018 at 06:27, Joel Stanley <joel@jms.id.au> wrote: >>>> >>>>>> /* Uart divisor latch read */ >>>>>> @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) >>>>>> UART_DA830_PWREMU_MGMT_FREE); >>>>>> } >>>>>> >>>>>> + if (port->type == PORT_NPCM) { >>>>>> + /* >>>>>> + * Nuvoton calls the scratch register 'UART_TOR' (timeout >>>>>> + * register). Enable it, and set TIOC (timeout interrupt >>>>>> + * comparator) to be 0x20 for correct operation. >>>>>> + */ >>>>>> + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); >>>>>> + } >>>>> >>>>> Can we set the TOR register dynamically (adding special IOCTL)? >>>> >>>> We would have to have a good reason to do so. >>>> >>>> Under what circumstances would a user need to change this dynamically? >>> >>> Tomer, I agree it is not important to change at run time. >> I agree Avi, >> but the only problem I see with it that if we will need in the future >> to TOR register dynamically (new SOC, client demand, etc.) >> Could it be done in the 8250 files? > > If we have a requirement in the future I suggest we decide how to > solve that problem then. Cross that bridge when we come to it. Sounds good > > >>> >>>> >>>>> >>>>>> + >>>>>> #ifdef CONFIG_SERIAL_8250_RSA >>>>>> /* >>>>>> * If this is an RSA port, see if we can kick it up to the >>>>>> @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, >>>>>> return quot_16 >> 4; >>>>>> } >>>>>> >>>>>> +/* Nuvoton NPCM UARTs have a custom divisor calculation */ >>>>>> +static unsigned int npcm_get_divisor(struct uart_8250_port *up, >>>>>> + unsigned int baud) >>>>>> +{ >>>>>> + struct uart_port *port = &up->port; >>>>>> + >>>>>> + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; >>>>>> +} >>>> >>>> Did you test this version of the driver on your hardware? >> Yes Thanks!!! it working fine. > > Great! > >>>> >>>> I think I understand why you had the extra code, but does it affect >>>> operation in any of your tests? >>>> >>>> From what I understand the baud rates are often calculated to be "near enough". >>> >>> Joel, I made the original calculation but I agree that this one is >>> good enough (I confused Tomer to think that your calculation will not >>> fit :) >>> >> I see you add the TOR to include/uapi/linux/serial_reg.h will the >> maintainer accept it? > > It's in a private include, include/uapi/linux/serial_reg.h. > > Initially I didn't do this, but if you examine the end of that file > there are a number of other 8250 ports that do this. > > If you would like, I will send this patch upstream now. Yes great please do, Thanks a lot > > Cheers, > > Joel Cheers, Tomer
diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt index 419ff6c0a47f..e8d921bae665 100644 --- a/Documentation/devicetree/bindings/serial/8250.txt +++ b/Documentation/devicetree/bindings/serial/8250.txt @@ -22,6 +22,7 @@ Required properties: - "ti,da830-uart" - "aspeed,ast2400-vuart" - "aspeed,ast2500-vuart" + - "nuvoton,npcm750-uart" - "serial" if the port type is unknown. - reg : offset and length of the register set for the device. - interrupts : should contain uart interrupt. diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c index 0cf95fddccfc..a947e1e36930 100644 --- a/drivers/tty/serial/8250/8250_of.c +++ b/drivers/tty/serial/8250/8250_of.c @@ -298,6 +298,7 @@ static const struct of_device_id of_platform_serial_table[] = { { .compatible = "mrvl,mmp-uart", .data = (void *)PORT_XSCALE, }, { .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, }, + { .compatible = "nuvoton,npcm750-uart", .data = (void *)PORT_NPCM, }, { /* end of list */ }, }; MODULE_DEVICE_TABLE(of, of_platform_serial_table); diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index a5fe0e66c607..df52f9ddbb99 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -289,6 +289,16 @@ static const struct serial8250_config uart_config[] = { .rxtrig_bytes = {1, 4, 8, 14}, .flags = UART_CAP_FIFO | UART_CAP_AFE, }, + [PORT_NPCM] = { + .name = "Nuvoton 16550", + .fifo_size = 16, + .tx_loadsz = 16, + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | + UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT, + .rxtrig_bytes = {1, 4, 8, 14}, + .flags = UART_CAP_FIFO, + + }, }; /* Uart divisor latch read */ @@ -2143,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port) UART_DA830_PWREMU_MGMT_FREE); } + if (port->type == PORT_NPCM) { + /* + * Nuvoton calls the scratch register 'UART_TOR' (timeout + * register). Enable it, and set TIOC (timeout interrupt + * comparator) to be 0x20 for correct operation. + */ + serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20); + } + #ifdef CONFIG_SERIAL_8250_RSA /* * If this is an RSA port, see if we can kick it up to the @@ -2465,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up, return quot_16 >> 4; } +/* Nuvoton NPCM UARTs have a custom divisor calculation */ +static unsigned int npcm_get_divisor(struct uart_8250_port *up, + unsigned int baud) +{ + struct uart_port *port = &up->port; + + return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2; +} + static unsigned int serial8250_get_divisor(struct uart_8250_port *up, unsigned int baud, unsigned int *frac) @@ -2485,6 +2513,8 @@ static unsigned int serial8250_get_divisor(struct uart_8250_port *up, quot = 0x8002; else if (up->port.type == PORT_XR17V35X) quot = xr17v35x_get_divisor(up, baud, frac); + else if (up->port.type == PORT_NPCM) + quot = npcm_get_divisor(up, baud); else quot = uart_get_divisor(port, baud); diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h index c34a2a3eeff5..4807dca14ea2 100644 --- a/include/uapi/linux/serial_core.h +++ b/include/uapi/linux/serial_core.h @@ -271,4 +271,7 @@ /* MPS2 UART */ #define PORT_MPS2UART 116 +/* Nuvton UART */ +#define PORT_NPCM 117 + #endif /* _UAPILINUX_SERIAL_CORE_H */ diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h index 5db76880b4ad..50fc96431eb5 100644 --- a/include/uapi/linux/serial_reg.h +++ b/include/uapi/linux/serial_reg.h @@ -374,5 +374,10 @@ #define UART_ALTR_EN_TXFIFO_LW 0x01 /* Enable the TX FIFO Low Watermark */ #define UART_ALTR_TX_LOW 0x41 /* Tx FIFO Low Watermark */ + +/* Nuvoton NPCM timeout register */ +#define UART_NPCM_TOR 7 +#define UART_NPCM_TOIE BIT(7) /* Timeout Interrupt Enable */ + #endif /* _LINUX_SERIAL_REG_H */