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
Permalink
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,
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;
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,
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;
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.
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,
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;
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.
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,
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;
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.
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
Loading...