[PATCH v4 0/2] serial: 8250_dw: Add ACPI support for uart on Hisilicon Hip05 SoC

[PATCH v4 0/2] serial: 8250_dw: Add ACPI support for uart on Hisilicon Hip05 SoC

[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

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

[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

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

[Heikki Krogerus]

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

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

[Heikki Krogerus]

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

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

[Kefeng Wang]

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

[PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC

[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

Re: [PATCH v4 2/2] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 SoC

[Heikki Krogerus]

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