[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits.

patch1 introduces necessary hooks to 8250 core.
patch2 implement the fractional divisor support for Synopsys DW 8250.

Jisheng Zhang (2):
  serial: 8250: introduce get_divisor() and set_divisor() hook
  serial: 8250_dw: add fractional divisor support

 drivers/tty/serial/8250/8250_core.c |  4 +++
 drivers/tty/serial/8250/8250_dw.c   | 54 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c |  8 +++++
 include/linux/serial_8250.h         |  7 ++++
 4 files changed, 73 insertions(+)

-- 
2.18.0

[PATCH 1/2] serial: 8250: introduce get_divisor() and set_divisor() hook

[Jisheng Zhang]

For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits. Add get_divisor() and set_divisor() hook to prepare
supporting this feature in next commit.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/tty/serial/8250/8250_core.c | 4 ++++
 drivers/tty/serial/8250/8250_port.c | 8 ++++++++
 include/linux/serial_8250.h         | 7 +++++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..da0f3849bfce 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1035,6 +1035,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 			uart->dl_read = up->dl_read;
 		if (up->dl_write)
 			uart->dl_write = up->dl_write;
+		if (up->get_divisor)
+			uart->get_divisor = up->get_divisor;
+		if (up->set_divisor)
+			uart->set_divisor = up->set_divisor;
 
 		if (uart->port.type != PORT_8250_CIR) {
 			if (serial8250_isa_config != NULL)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index cf541aab2bd0..0060ec54a522 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2505,6 +2505,9 @@ static unsigned int serial8250_get_divisor(struct uart_8250_port *up,
 	struct uart_port *port = &up->port;
 	unsigned int quot;
 
+	if (up->get_divisor)
+		return up->get_divisor(up, baud, frac);
+
 	/*
 	 * Handle magic divisors for baud rates above baud_base on
 	 * SMSC SuperIO chips.
@@ -2575,6 +2578,11 @@ static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
+	if (up->set_divisor) {
+		up->set_divisor(up, baud, quot, quot_frac);
+		return;
+	}
+
 	/* Workaround to enable 115200 baud on OMAP1510 internal ports */
 	if (is_omap1510_8250(up)) {
 		if (baud == 115200) {
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 76b9db71e489..5093c9ca0980 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -132,6 +132,13 @@ struct uart_8250_port {
 	/* 8250 specific callbacks */
 	int			(*dl_read)(struct uart_8250_port *);
 	void			(*dl_write)(struct uart_8250_port *, int);
+	unsigned int		(*get_divisor)(struct uart_8250_port *up,
+					       unsigned int baud,
+					       unsigned int *frac);
+	void			(*set_divisor)(struct uart_8250_port *up,
+					       unsigned int baud,
+					       unsigned int quot,
+					       unsigned int quot_frac);
 
 	struct uart_8250_em485 *em485;
 };
-- 
2.18.0

[PATCH 2/2] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits.

Now the preparation is done, it's easy to add the feature support.
This patch firstly checks the component version during probe, if
version >= 4.00a, then calculates the fractional divisor width, then
setups dw specific get_divisor() and set_divisor() hook.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/tty/serial/8250/8250_dw.c | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index aff04f1de3a5..367a73507c5e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -31,9 +31,12 @@
 
 /* Offsets for the DesignWare specific registers */
 #define DW_UART_USR	0x1f /* UART Status Register */
+#define DW_UART_DLF	0xc0 /* Divisor Latch Fraction Register */
 #define DW_UART_CPR	0xf4 /* Component Parameter Register */
 #define DW_UART_UCV	0xf8 /* UART Component Version */
 
+#define DW_FRAC_MIN_VERS		0x3430302A
+
 /* Component Parameter Register bits */
 #define DW_UART_CPR_ABP_DATA_WIDTH	(3 << 0)
 #define DW_UART_CPR_AFCE_MODE		(1 << 4)
@@ -65,6 +68,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		dlf_size:3;
 };
 
 static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
@@ -351,6 +355,34 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
 	return param == chan->device->dev->parent;
 }
 
+/*
+ * For version >= 4.00a, the dw uarts have a valid divisor latch fraction
+ * register. Calculate divisor with extra 4bits ~ 6bits fractional portion.
+ */
+static unsigned int dw8250_get_divisor(struct uart_8250_port *up,
+				       unsigned int baud,
+				       unsigned int *frac)
+{
+	unsigned int quot;
+	struct uart_port *p = &up->port;
+	struct dw8250_data *d = p->private_data;
+
+	quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4), baud);
+	*frac = quot & (~0U >> (32 - d->dlf_size));
+
+	return quot >> d->dlf_size;
+}
+
+static void dw8250_set_divisor(struct uart_8250_port *up, unsigned int baud,
+			       unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_port *p = &up->port;
+
+	serial_port_out(p, DW_UART_DLF >> p->regshift, quot_frac);
+	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+	serial_dl_write(up, quot);
+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	if (p->dev->of_node) {
@@ -414,6 +446,28 @@ static void dw8250_setup_port(struct uart_port *p)
 	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
+	/*
+	 * For version >= 4.00a, the dw uarts have a valid divisor latch
+	 * fraction register. Calculate the fractional divisor width.
+	 */
+	if (reg >= DW_FRAC_MIN_VERS) {
+		struct dw8250_data *d = p->private_data;
+
+		if (p->iotype == UPIO_MEM32BE) {
+			iowrite32be(~0U, p->membase + DW_UART_DLF);
+			reg = ioread32be(p->membase + DW_UART_DLF);
+		} else {
+			writel(~0U, p->membase + DW_UART_DLF);
+			reg = readl(p->membase + DW_UART_DLF);
+		}
+		d->dlf_size = fls(reg);
+
+		if (d->dlf_size) {
+			up->get_divisor = dw8250_get_divisor;
+			up->set_divisor = dw8250_set_divisor;
+		}
+	}
+
 	if (p->iotype == UPIO_MEM32BE)
 		reg = ioread32be(p->membase + DW_UART_CPR);
 	else
-- 
2.18.0

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Mon, 2018-07-02 at 18:04 +0800, Jisheng Zhang wrote:
> For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
> valid divisor latch fraction register. The fractional divisor width is
> 4bits ~ 6bits.
> 

There are several serial IPs that have fractional divider built-in. None
is using any specific hooks. Why do you need in your case, esp. taking
into consideration that we have a custom ->set_termios() callback?

> patch1 introduces necessary hooks to 8250 core.
> patch2 implement the fractional divisor support for Synopsys DW 8250.
> 
> Jisheng Zhang (2):
>   serial: 8250: introduce get_divisor() and set_divisor() hook
>   serial: 8250_dw: add fractional divisor support
> 
>  drivers/tty/serial/8250/8250_core.c |  4 +++
>  drivers/tty/serial/8250/8250_dw.c   | 54
> +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/8250_port.c |  8 +++++
>  include/linux/serial_8250.h         |  7 ++++
>  4 files changed, 73 insertions(+)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Mon, 2018-07-02 at 13:18 +0300, Andy Shevchenko wrote:
> On Mon, 2018-07-02 at 18:04 +0800, Jisheng Zhang wrote:
> > For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
> > valid divisor latch fraction register. The fractional divisor width
> > is
> > 4bits ~ 6bits.
> > 
> 
> There are several serial IPs that have fractional divider built-in.
> None
> is using any specific hooks. Why do you need in your case, esp. taking
> into consideration that we have a custom ->set_termios() callback?

Okay, I see that in 8250 we have hooks embedded into 8250_port.c which
is not the best solution.

For example it prevents better splitting Exar code.
So, we would need these hooks, but better to integrate them in the same
way like it's done for the rest of 8250 ones, i.e.
- rename existing to have a "do" word
- create new functions which would be a replacement that choose between
"do" variant and custom one
- not sure if we need to export "do" variants (at least for now)

> 
> > patch1 introduces necessary hooks to 8250 core.
> > patch2 implement the fractional divisor support for Synopsys DW
> > 8250.
> > 
> > Jisheng Zhang (2):
> >   serial: 8250: introduce get_divisor() and set_divisor() hook
> >   serial: 8250_dw: add fractional divisor support
> > 
> >  drivers/tty/serial/8250/8250_core.c |  4 +++
> >  drivers/tty/serial/8250/8250_dw.c   | 54
> > +++++++++++++++++++++++++++++
> >  drivers/tty/serial/8250/8250_port.c |  8 +++++
> >  include/linux/serial_8250.h         |  7 ++++
> >  4 files changed, 73 insertions(+)
> > 
> 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

Hi,

On Mon, 2 Jul 2018 14:51:03 +0300 Andy Shevchenko wrote:

> On Mon, 2018-07-02 at 13:18 +0300, Andy Shevchenko wrote:
> > On Mon, 2018-07-02 at 18:04 +0800, Jisheng Zhang wrote:  
> > > For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
> > > valid divisor latch fraction register. The fractional divisor width
> > > is
> > > 4bits ~ 6bits.
> > >   
> > 
> > There are several serial IPs that have fractional divider built-in.
> > None
> > is using any specific hooks. Why do you need in your case, esp. taking
> > into consideration that we have a custom ->set_termios() callback?  
> 
> Okay, I see that in 8250 we have hooks embedded into 8250_port.c which
> is not the best solution.
> 
> For example it prevents better splitting Exar code.
> So, we would need these hooks, but better to integrate them in the same
> way like it's done for the rest of 8250 ones, i.e.
> - rename existing to have a "do" word
> - create new functions which would be a replacement that choose between
> "do" variant and custom one
> - not sure if we need to export "do" variants (at least for now)

So you mean add the support as following:

1.rename current serial8250_set_divisor as serial8250_do_set_divisor

2.add a new serial8250_set_divisor which will be as simple as

static void serial8250_set_divisor(struct uart_port *port, ...)
{
	struct uart_8250_port *up = up_to_u8250p(port);

	if (up->set_divisor)
		up->set_divisor(...);
	else
		serial8250_do_set_divisor(...);
}

could you please confirm?

Another issue is I'm not sure which struct to add the hook,
struct uart_port or struct uart_8250_port? Currently, it seems that only
uart_8250_port needs this hook, sure, adding the hook to struct uart_port
is fine either. Could you please kindly give some suggestions?

Thanks,
Jisheng

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

On Tue, 3 Jul 2018 10:22:57 +0800 Jisheng Zhang wrote:

> Hi,
> 
> On Mon, 2 Jul 2018 14:51:03 +0300 Andy Shevchenko wrote:
> 
> > On Mon, 2018-07-02 at 13:18 +0300, Andy Shevchenko wrote:  
> > > On Mon, 2018-07-02 at 18:04 +0800, Jisheng Zhang wrote:    
> > > > For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
> > > > valid divisor latch fraction register. The fractional divisor width
> > > > is
> > > > 4bits ~ 6bits.
> > > >     
> > > 
> > > There are several serial IPs that have fractional divider built-in.
> > > None
> > > is using any specific hooks. Why do you need in your case, esp. taking
> > > into consideration that we have a custom ->set_termios() callback?    
> > 
> > Okay, I see that in 8250 we have hooks embedded into 8250_port.c which
> > is not the best solution.
> > 
> > For example it prevents better splitting Exar code.
> > So, we would need these hooks, but better to integrate them in the same
> > way like it's done for the rest of 8250 ones, i.e.
> > - rename existing to have a "do" word
> > - create new functions which would be a replacement that choose between
> > "do" variant and custom one
> > - not sure if we need to export "do" variants (at least for now)  
> 
> So you mean add the support as following:
> 
> 1.rename current serial8250_set_divisor as serial8250_do_set_divisor
> 
> 2.add a new serial8250_set_divisor which will be as simple as
> 
> static void serial8250_set_divisor(struct uart_port *port, ...)
> {
> 	struct uart_8250_port *up = up_to_u8250p(port);
> 
> 	if (up->set_divisor)
> 		up->set_divisor(...);
> 	else
> 		serial8250_do_set_divisor(...);
> }
> 
> could you please confirm?
> 
> Another issue is I'm not sure which struct to add the hook,
> struct uart_port or struct uart_8250_port? Currently, it seems that only
> uart_8250_port needs this hook, sure, adding the hook to struct uart_port
> is fine either. Could you please kindly give some suggestions?

patching struct uart_port seems a bit overhead. After reading the code
again, I propose another solution, similar as what dl_write() is used in
8250 core:

1.introduce the hook to struct uart_8250_port as my previous patches do, 

2.rename current serial8250_set_divisor() as default_serial_get_divisor()
then introduce a new serial8250_set_divisor() as:
static inline void serial8250_set_divisor(struct uart_8250_port *up,....)
{
	up->set_divisor();
}

and point up->set-divisor to default_serial_get_divisor

what do you think about this solution?

Thanks

> 
> Thanks,
> Jisheng

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Tue, 2018-07-03 at 10:48 +0800, Jisheng Zhang wrote:
> On Tue, 3 Jul 2018 10:22:57 +0800 Jisheng Zhang wrote:

> patching struct uart_port seems a bit overhead. After reading the code
> again, I propose another solution, similar as what dl_write() is used
> in
> 8250 core:
> 
> 1.introduce the hook to struct uart_8250_port as my previous patches
> do, 
> 
> 2.rename current serial8250_set_divisor() as
> default_serial_get_divisor()
> then introduce a new serial8250_set_divisor() as:
> static inline void serial8250_set_divisor(struct uart_8250_port
> *up,....)
> {
> 	up->set_divisor();
> }
> 
> and point up->set-divisor to default_serial_get_divisor
> 
> what do you think about this solution?

Disagree. See my previous answer for details.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 0/2] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Tue, 2018-07-03 at 10:22 +0800, Jisheng Zhang wrote:
> Hi,
> 
> On Mon, 2 Jul 2018 14:51:03 +0300 Andy Shevchenko wrote:
> 
> > On Mon, 2018-07-02 at 13:18 +0300, Andy Shevchenko wrote:
> > > On Mon, 2018-07-02 at 18:04 +0800, Jisheng Zhang wrote:  
> > > > For Synopsys DesignWare 8250 uart which version >= 4.00a,
> > > > there's a
> > > > valid divisor latch fraction register. The fractional divisor
> > > > width
> > > > is
> > > > 4bits ~ 6bits.
> > > >   
> > > 
> > > There are several serial IPs that have fractional divider built-
> > > in.
> > > None
> > > is using any specific hooks. Why do you need in your case, esp.
> > > taking
> > > into consideration that we have a custom ->set_termios()
> > > callback?  
> > 
> > Okay, I see that in 8250 we have hooks embedded into 8250_port.c
> > which
> > is not the best solution.
> > 
> > For example it prevents better splitting Exar code.
> > So, we would need these hooks, but better to integrate them in the
> > same
> > way like it's done for the rest of 8250 ones, i.e.
> > - rename existing to have a "do" word
> > - create new functions which would be a replacement that choose
> > between
> > "do" variant and custom one
> > - not sure if we need to export "do" variants (at least for now)
> 
> So you mean add the support as following:
> 
> 1.rename current serial8250_set_divisor as serial8250_do_set_divisor
> 

Yes

> 2.add a new serial8250_set_divisor which will be as simple as

Yes and no. 

> 
> static void serial8250_set_divisor(struct uart_port *port, ...)
> {
> 	struct uart_8250_port *up = up_to_u8250p(port);
> 
> 	if (up->set_divisor)

port->

> 		up->set_divisor(...);

port->

> 	else
> 		serial8250_do_set_divisor(...);
> }
> 
> could you please confirm?

Yes.

> 
> Another issue is I'm not sure which struct to add the hook,
> struct uart_port or struct uart_8250_port? Currently, it seems that
> only
> uart_8250_port needs this hook,

Define "needs". 8250 _uses_ it, the rest which I easy grepped would be
able to use it if we provide such a possibility.

lantiq.c is one example (currently they don't use it), or
drivers/tty/serial/digicolor-usart.c limits baud rate choice b/c of
absence of a common way.

>  sure, adding the hook to struct uart_port
> is fine either. Could you please kindly give some suggestions?

See above. Thanks for doing this!

> 
> Thanks,
> Jisheng

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy