[PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set

Discussion:

[PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback

(too old to reply)

Kefeng Wang

2016-08-31 03:31:37 UTC

Make dw8250_set_termios() handle all cases of dw8250_data->clk properly,
then we can safely use dw8250_set_termios() as the default set_termios
callback instead of serial8250_do_set_termios(), so do it.

Signed-off-by: Kefeng Wang <***@huawei.com>
---
drivers/tty/serial/8250/8250_dw.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 5c0c123..adecece 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -235,7 +235,7 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
unsigned int rate;
int ret;

- if (IS_ERR(d->clk) || !old)
+ if (IS_ERR_OR_NULL(d->clk) || !old)
goto out;

clk_disable_unprepare(d->clk);
@@ -308,13 +308,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
p->serial_in = dw8250_serial_in32;
data->uart_16550_compatible = true;
}
- p->set_termios = dw8250_set_termios;
}

/* Platforms with iDMA */
if (platform_get_resource_byname(to_platform_device(p->dev),
IORESOURCE_MEM, "lpss_priv")) {
- p->set_termios = dw8250_set_termios;
data->dma.rx_param = p->dev->parent;
data->dma.tx_param = p->dev->parent;
data->dma.fn = dw8250_idma_filter;
@@ -391,6 +389,7 @@ static int dw8250_probe(struct platform_device *pdev)
p->iotype = UPIO_MEM;
p->serial_in = dw8250_serial_in;
p->serial_out = dw8250_serial_out;
+ p->set_termios = dw8250_set_termios;

p->membase = devm_ioremap(&pdev->dev, regs->start, resource_size(regs));
if (!p->membase)

--
1.7.12.4

Kefeng Wang

2016-08-31 03:34:15 UTC

Permalink

Add ACPI identifier for UART on Hisilicon Hip05 SoC, be careful that
it is not 16550 compatible, and "reg-io-width" and "reg-shift" need
be set properly by _DSD method in DSDT.

Signed-off-by: Kefeng Wang <***@huawei.com>
---
drivers/tty/serial/8250/8250_dw.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index adecece..3478c2c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -623,6 +623,7 @@ static const struct acpi_device_id dw8250_acpi_match[] = {
{ "APMC0D08", 0},
{ "AMD0020", 0 },
{ "AMDI0020", 0 },
+ { "HISI0031", 0 },
{ },
};
MODULE_DEVICE_TABLE(acpi, dw8250_acpi_match);

--
1.7.12.4

Heikki Krogerus

2016-09-02 11:16:16 UTC

Permalink

Post by Kefeng Wang
Add ACPI identifier for UART on Hisilicon Hip05 SoC, be careful that
it is not 16550 compatible, and "reg-io-width" and "reg-shift" need
be set properly by _DSD method in DSDT.

Acked-by: Heikki Krogerus <***@linux.intel.com>

Thanks,

--
heikki

Heikki Krogerus

2016-08-31 12:08:11 UTC

Permalink

Hi,

This part is fine, but...

Post by Kefeng Wang
clk_disable_unprepare(d->clk);
@@ -308,13 +308,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
p->serial_in = dw8250_serial_in32;
data->uart_16550_compatible = true;
}
- p->set_termios = dw8250_set_termios;
}
/* Platforms with iDMA */
if (platform_get_resource_byname(to_platform_device(p->dev),
IORESOURCE_MEM, "lpss_priv")) {
- p->set_termios = dw8250_set_termios;
data->dma.rx_param = p->dev->parent;
data->dma.tx_param = p->dev->parent;
data->dma.fn = dw8250_idma_filter;
@@ -391,6 +389,7 @@ static int dw8250_probe(struct platform_device *pdev)
p->iotype = UPIO_MEM;
p->serial_in = dw8250_serial_in;
p->serial_out = dw8250_serial_out;
+ p->set_termios = dw8250_set_termios;

I'm a bit worried about this. If I remember correctly, we didn't set
the set_termios hook by default in the past because somebody was using
a type of clk that can't handle rate rounding and ended up using
improper clock frequency for some baud rates. Unfortunately I can't
remember who had that problem and on which platform.

On the other hand, I'm not sure if drivers should worry about things
like that. This should be done the other way around. The platform
with the problem should unset the p->set_termios hook in a quirk, but
by default everybody would use dw8250_set_termios.

Thanks,

--
heikki

Heikki Krogerus

2016-09-02 11:27:40 UTC

Permalink

Post by Heikki Krogerus
Hi,

This part is fine, but...

I'm a bit worried about this. If I remember correctly, we didn't set
the set_termios hook by default in the past because somebody was using
a type of clk that can't handle rate rounding and ended up using
improper clock frequency for some baud rates. Unfortunately I can't
remember who had that problem and on which platform.
On the other hand, I'm not sure if drivers should worry about things
like that. This should be done the other way around. The platform
with the problem should unset the p->set_termios hook in a quirk, but
by default everybody would use dw8250_set_termios.

No comments... I would suggest we skip this for now. I don't think
it's necessary for Hisilicon Hip05 SoC support, so perhaps you can
just propose this change separately.

Thanks,

--
heikki

Kefeng Wang

2016-09-05 01:29:51 UTC

Permalink

Post by Heikki Krogerus

Post by Heikki Krogerus
Hi,

This part is fine, but...

I'm a bit worried about this. If I remember correctly, we didn't set
the set_termios hook by default in the past because somebody was using
a type of clk that can't handle rate rounding and ended up using
improper clock frequency for some baud rates. Unfortunately I can't
remember who had that problem and on which platform.
On the other hand, I'm not sure if drivers should worry about things
like that. This should be done the other way around. The platform
with the problem should unset the p->set_termios hook in a quirk, but
by default everybody would use dw8250_set_termios.

No comments... I would suggest we skip this for now. I don't think
it's necessary for Hisilicon Hip05 SoC support, so perhaps you can
just propose this change separately.

OK, will send it separately, like you said, if some platform has issue
with this change, let's work around it in dw8250_quirks().

Thanks,
Kefeng

u***@gmail.com

2016-11-16 23:55:14 UTC

Permalink

Hi Kefeng,

I was wondering if you can share your solution in the quirks to address this. I have a system that needs to use hardware CTS/RTS and I need to call the dw8250_set_termios so the proper flag can be set. But I'm not sure what's the best way to add this to the quirk function.

Regards,
Jason

Post by Kefeng Wang

Post by Heikki Krogerus

Post by Heikki Krogerus
Hi,

This part is fine, but...

I'm a bit worried about this. If I remember correctly, we didn't set
the set_termios hook by default in the past because somebody was using
a type of clk that can't handle rate rounding and ended up using
improper clock frequency for some baud rates. Unfortunately I can't
remember who had that problem and on which platform.
On the other hand, I'm not sure if drivers should worry about things
like that. This should be done the other way around. The platform
with the problem should unset the p->set_termios hook in a quirk, but
by default everybody would use dw8250_set_termios.

No comments... I would suggest we skip this for now. I don't think
it's necessary for Hisilicon Hip05 SoC support, so perhaps you can
just propose this change separately.

OK, will send it separately, like you said, if some platform has issue
with this change, let's work around it in dw8250_quirks().
Thanks,
Kefeng

AliveisVivo

2017-05-03 09:00:49 UTC

Permalink

Post by Kefeng Wang
Make dw8250_set_termios() handle all cases of dw8250_data->clk properly,
then we can safely use dw8250_set_termios() as the default set_termios
callback instead of serial8250_do_set_termios(), so do it.
---
drivers/tty/serial/8250/8250_dw.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 5c0c123..adecece 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -235,7 +235,7 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
unsigned int rate;
int ret;
- if (IS_ERR(d->clk) || !old)
+ if (IS_ERR_OR_NULL(d->clk) || !old)
goto out;
clk_disable_unprepare(d->clk);
@@ -308,13 +308,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
p->serial_in = dw8250_serial_in32;
data->uart_16550_compatible = true;
}
- p->set_termios = dw8250_set_termios;
}
/* Platforms with iDMA */
if (platform_get_resource_byname(to_platform_device(p->dev),
IORESOURCE_MEM, "lpss_priv")) {
- p->set_termios = dw8250_set_termios;
data->dma.rx_param = p->dev->parent;
data->dma.tx_param = p->dev->parent;
adata->dma.fn = dw8250_idma_filter;
@@ -391,6 +389,7 @@ static int dw8250_probe(struct platform_device *pdev)
p->iotype = UPIO_MEM;
p->serial_in = dw8250_serial_in;
p->serial_out = dw8250_serial_out;
+ p->set_termios = dw8250_set_termios;
p->membase = devm_ioremap(&pdev->dev, regs->start, resource_size(regs));
if (!p->membase)
--
1.7.12.4