* [PATCH v4 0/2] serial: 8250_dw: Add ACPI support for uart on Hisilicon Hip05 SoC
@ 2016-08-31 3:29 Kefeng Wang
2016-08-31 3:29 ` [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback Kefeng Wang
2016-08-31 3:29 ` [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
0 siblings, 2 replies; 7+ messages in thread
From: Kefeng Wang @ 2016-08-31 3:29 UTC (permalink / raw)
To: gregkh, linux-serial, linux-kernel
Cc: Andy Shevchenko, Heikki Krogerus, Jiri Slaby, guohanjun, xuwei5,
graeme.gregory, z.liuxinliang, linux-acpi, Kefeng Wang
Handle all cases of dw8250_data->clk properly in dw8250_set_termios(), then
make it as the default set_termios callback for 8250 dw uart.
After that, add ACPI support for uart on Hisilicon Hip05 SoC, be careful that
it is not 16500 compatible, and with Heikki's patchset[1], we safely only add
ACPI identifier, due to the ACPI quirks in dw8250_quirks() only for "APMC0D08".
Change since v3:
- The new dev var patch is sent separately.
- Address the comment from Andy, and repost patches based on Heikki Krogerus's patchset[1]
"[PATCHv2 0/3] serial: dw8250: ACPI tuning"
Change since v2:
- Add a new patch to use new var dev in probe
- Use built-in device properties to set device parameters for existing device probed by acpi,
suggested by Andy Shevchenko
Change since v1:
- Use acpi_match_device() instead of acpi_dev_found(), limit the check to the device
being probed and not a global search for whole DSDT (pointed by graeme.gregory@linaro.org)
[1] http://www.spinics.net/lists/linux-acpi/msg68519.html
Kefeng Wang (2):
serial: 8250_dw: make dw8250_set_termios as default set_termios
callback
serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC
drivers/tty/serial/8250/8250_dw.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--
1.7.12.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback
2016-08-31 3:29 [PATCH v4 0/2] serial: 8250_dw: Add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
@ 2016-08-31 3:29 ` Kefeng Wang
2016-08-31 12:07 ` Heikki Krogerus
2016-08-31 3:29 ` [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
1 sibling, 1 reply; 7+ messages in thread
From: Kefeng Wang @ 2016-08-31 3:29 UTC (permalink / raw)
To: gregkh, linux-serial, linux-kernel
Cc: Andy Shevchenko, Heikki Krogerus, Jiri Slaby, guohanjun, xuwei5,
graeme.gregory, z.liuxinliang, linux-acpi, 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.
Signed-off-by: Kefeng Wang <wangkefeng.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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC
2016-08-31 3:29 [PATCH v4 0/2] serial: 8250_dw: Add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
2016-08-31 3:29 ` [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback Kefeng Wang
@ 2016-08-31 3:29 ` Kefeng Wang
2016-09-02 11:16 ` Heikki Krogerus
1 sibling, 1 reply; 7+ messages in thread
From: Kefeng Wang @ 2016-08-31 3:29 UTC (permalink / raw)
To: gregkh, linux-serial, linux-kernel
Cc: Andy Shevchenko, Heikki Krogerus, Jiri Slaby, guohanjun, xuwei5,
graeme.gregory, z.liuxinliang, linux-acpi, 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.
Signed-off-by: Kefeng Wang <wangkefeng.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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback
2016-08-31 3:29 ` [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback Kefeng Wang
@ 2016-08-31 12:07 ` Heikki Krogerus
2016-09-02 11:21 ` Heikki Krogerus
0 siblings, 1 reply; 7+ messages in thread
From: Heikki Krogerus @ 2016-08-31 12:07 UTC (permalink / raw)
To: Kefeng Wang
Cc: gregkh, linux-serial, linux-kernel, Andy Shevchenko, Jiri Slaby,
guohanjun, xuwei5, graeme.gregory, z.liuxinliang, linux-acpi
Hi,
On Wed, Aug 31, 2016 at 11:29:11AM +0800, Kefeng Wang wrote:
> 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 <wangkefeng.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;
This part is fine, but...
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC
2016-08-31 3:29 ` [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
@ 2016-09-02 11:16 ` Heikki Krogerus
0 siblings, 0 replies; 7+ messages in thread
From: Heikki Krogerus @ 2016-09-02 11:16 UTC (permalink / raw)
To: Kefeng Wang
Cc: gregkh, linux-serial, linux-kernel, Andy Shevchenko, Jiri Slaby,
guohanjun, xuwei5, graeme.gregory, z.liuxinliang, linux-acpi
On Wed, Aug 31, 2016 at 11:29:12AM +0800, Kefeng Wang wrote:
> 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 <wangkefeng.wang@huawei.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Thanks,
--
heikki
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback
2016-08-31 12:07 ` Heikki Krogerus
@ 2016-09-02 11:21 ` Heikki Krogerus
2016-09-05 1:28 ` Kefeng Wang
0 siblings, 1 reply; 7+ messages in thread
From: Heikki Krogerus @ 2016-09-02 11:21 UTC (permalink / raw)
To: Kefeng Wang
Cc: gregkh, linux-serial, linux-kernel, Andy Shevchenko, Jiri Slaby,
guohanjun, xuwei5, graeme.gregory, z.liuxinliang, linux-acpi
On Wed, Aug 31, 2016 at 03:07:56PM +0300, Heikki Krogerus wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 11:29:11AM +0800, Kefeng Wang wrote:
> > 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 <wangkefeng.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;
>
> This part is fine, but...
>
> > 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback
2016-09-02 11:21 ` Heikki Krogerus
@ 2016-09-05 1:28 ` Kefeng Wang
0 siblings, 0 replies; 7+ messages in thread
From: Kefeng Wang @ 2016-09-05 1:28 UTC (permalink / raw)
To: Heikki Krogerus
Cc: gregkh, linux-serial, linux-kernel, Andy Shevchenko, Jiri Slaby,
guohanjun, xuwei5, graeme.gregory, z.liuxinliang, linux-acpi
On 2016/9/2 19:21, Heikki Krogerus wrote:
> On Wed, Aug 31, 2016 at 03:07:56PM +0300, Heikki Krogerus wrote:
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 11:29:11AM +0800, Kefeng Wang wrote:
>>> 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 <wangkefeng.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;
>>
>> This part is fine, but...
>>
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-05 1:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-31 3:29 [PATCH v4 0/2] serial: 8250_dw: Add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
2016-08-31 3:29 ` [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback Kefeng Wang
2016-08-31 12:07 ` Heikki Krogerus
2016-09-02 11:21 ` Heikki Krogerus
2016-09-05 1:28 ` Kefeng Wang
2016-08-31 3:29 ` [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC Kefeng Wang
2016-09-02 11:16 ` Heikki Krogerus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).