RE: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[Wang, Annie]

Hi Heikki,

>-----Original Message-----
>From: Heikki Krogerus [mailto:heikki.krogerus@xxxxxxxxxxxxxxx]
>Sent: Tuesday, January 05, 2016 8:02 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
>serial@xxxxxxxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx; Borislav Petkov; Huang,
>Ray; Wan, Vincent; Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu,
>Xiangliang
>Subject: Re: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250
>
>Hi Hongcheng,
>
>My comments below..
>
>On Mon, Jan 04, 2016 at 01:31:41PM +0800, Wang Hongcheng wrote:
>> Set a new port type for AMD Carrizo.  Add has_pl330_dma to 8250_dw's
>> private data and init fcr,ier as well as dma rx size.
>>
>> Signed-off-by: Wang Hongcheng <annie.wang@xxxxxxx>
>> ---
>>  drivers/acpi/acpi_apd.c               | 11 +++++++++++
>>  drivers/tty/serial/8250/8250_dw.c     | 15 +++++++++++++++
>>  drivers/tty/serial/8250/8250_port.c   |  9 +++++++++
>>  include/linux/platform_data/8250-dw.h |  8 ++++++++
>>  include/uapi/linux/serial_core.h      |  3 ++-
>>  include/uapi/linux/serial_reg.h       |  1 +
>>  6 files changed, 46 insertions(+), 1 deletion(-)  create mode 100644
>> include/linux/platform_data/8250-dw.h
>>
>> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c index
>> 1f16cca..e8e43fb 100644
>> --- a/drivers/acpi/acpi_apd.c
>> +++ b/drivers/acpi/acpi_apd.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/dmaengine.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/amba/pl330.h>
>> +#include <linux/platform_data/8250-dw.h>
>>
>>  #include "internal.h"
>>
>> @@ -56,6 +57,11 @@ static struct dma_pl330_platdata amd_pl330[] = {
>>  		.num = 0,
>>  	}
>>  };
>> +
>> +static struct plat_dw8250_data amd_dw8250 = {
>> +	.has_pl330_dma = 1,
>> +};
>
>Why do you need this? Can't you identify this platform in 8250_dw.c based on the
>ACPI ID?
>
>>  /**
>>   * struct apd_device_desc - a descriptor for apd device
>>   * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM @@ -115,6
>> +121,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>>  	char amba_devname[100];
>>  	int devnum;
>>
>> +	ret = platform_device_add_data(pdev, &amd_dw8250,
>> +				       sizeof(amd_dw8250));
>> +	if (ret)
>> +		goto resource_alloc_err;
>> +
>>  	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>>  	if (!resource)
>>  		goto resource_alloc_err;
>
>The above needs to be separated to its own patch in any case.
>
>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>> b/drivers/tty/serial/8250/8250_dw.c
>> index a5d319e..e7d9f01 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/reset.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/platform_data/8250-dw.h>
>>
>>  #include <asm/byteorder.h>
>>
>> @@ -66,6 +67,7 @@ struct dw8250_data {
>>
>>  	unsigned int		skip_autocfg:1;
>>  	unsigned int		uart_16550_compatible:1;
>> +	unsigned int		has_pl330_dma:1;
>
>I don't think there is any use for this flag. You can do all the platform specific
>tricks in a single quirk that you add to dw8250_quirks()..
>
>>  };
>>
>>  #define BYT_PRV_CLK			0x800
>> @@ -303,6 +305,7 @@ static void dw8250_quirks(struct uart_port *p,
>> struct dw8250_data *data)  static void dw8250_setup_port(struct
>> uart_port *p)  {
>>  	struct uart_8250_port *up = up_to_u8250p(p);
>> +	struct dw8250_data *data = p->private_data;
>>  	u32 reg;
>>
>>  	/*
>> @@ -326,6 +329,14 @@ static void dw8250_setup_port(struct uart_port *p)
>>  		p->flags |= UPF_FIXED_TYPE;
>>  		p->fifosize = DW_UART_CPR_FIFO_SIZE(reg);
>>  		up->capabilities = UART_CAP_FIFO;
>> +		if (data->has_pl330_dma) {
>> +			p->type = PORT_AMD_8250;
>> +
>> +			up->ier |= UART_IER_PTIME | UART_IER_THRI |
>> +				UART_IER_RLSI | UART_IER_RDI;
>> +			up->fcr |= UART_FCR_R_TRIG_10 |
>UART_FCR_T_TRIG_11;
>> +			up->tx_loadsz = p->fifosize / 2;
>
>So these need to be set in the quirk. Btw, now up->ier and up->fcr will just be
>forgotten when serial8250_register_8250_port() is called.
>
>The UART_IER_PTIME needs to be set based on THRE_MODE. Add a flag for that
>instead of the "has_pl330_dma", and set it in this function based on the
>THRE_MODE bit in CPR. Then I think the correct place to fix
>up->ier is actually our up->set_termios hook.
>
>> +		}
>>  	}
>>
>>  	if (reg & DW_UART_CPR_AFCE_MODE)
>> @@ -339,6 +350,7 @@ static int dw8250_probe(struct platform_device *pdev)
>>  	int irq = platform_get_irq(pdev, 0);
>>  	struct uart_port *p = &uart.port;
>>  	struct dw8250_data *data;
>> +	struct plat_dw8250_data *pdata = dev_get_platdata(&pdev->dev);
>>  	int err;
>>  	u32 val;
>>
>> @@ -468,6 +480,7 @@ static int dw8250_probe(struct platform_device *pdev)
>>  		p->handle_irq = NULL;
>>  	}
>>
>> +	data->has_pl330_dma = pdata ? pdata->has_pl330_dma : 0;
>>  	if (!data->skip_autocfg)
>>  		dw8250_setup_port(p);
>>
>> @@ -475,6 +488,8 @@ static int dw8250_probe(struct platform_device *pdev)
>>  	if (p->fifosize) {
>>  		data->dma.rxconf.src_maxburst = p->fifosize / 4;
>>  		data->dma.txconf.dst_maxburst = p->fifosize / 4;
>> +		data->dma.rx_size
>> +			= data->has_pl330_dma ? (p->fifosize / 2 + 2) : 0;
>
>This you also need to set in the quirk.

Yes, I wil set that in the quirk.

>
>>  		uart.dma = &data->dma;
>>  	}
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c
>> b/drivers/tty/serial/8250/8250_port.c
>> index 52d82d2..b89a326 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -269,6 +269,15 @@ configured less than Maximum supported fifo bytes */
>>  		.rxtrig_bytes	= {1, 4, 8, 14},
>>  		.flags		= UART_CAP_FIFO,
>>  	},
>> +	[PORT_AMD_8250] = {
>> +		.name		= "AMD_8250",
>> +		.fifo_size	= 256,
>> +		.tx_loadsz	= 128,
>> +		.fcr		= UART_FCR_ENABLE_FIFO |
>UART_FCR_R_TRIG_10 |
>> +				UART_FCR_T_TRIG_11,
>> +		.rxtrig_bytes	= {1, 4, 8},
>> +		.flags		= UART_CAP_FIFO,
>> +	},
>
>There is no use for this new type. In fact, there is no use for any new types. If we
>need to modify fcr for example, we can do that in a custom up->startup or up-
>>set_termios hook.
>
>>  };
>>
>>  /* Uart divisor latch read */
>> diff --git a/include/linux/platform_data/8250-dw.h
>> b/include/linux/platform_data/8250-dw.h
>> new file mode 100644
>> index 0000000..97cdb9d
>> --- /dev/null
>> +++ b/include/linux/platform_data/8250-dw.h
>> @@ -0,0 +1,8 @@
>> +#ifndef _PLATFORM_DATA_8250_DW_H
>> +#define _PLATFORM_DATA_8250_DW_H
>> +
>> +struct plat_dw8250_data {
>> +	unsigned int has_pl330_dma:1;
>> +};
>> +
>> +#endif
>
>Let's not add any new driver specific platform data files unless we absolutely have
>to. In this case there is no need for it. If you really need to deliver this kind of
>detail to the driver, you need to deliver it to the driver with build-in device
>property. So instead of
>platform_device_add_data() you should use device_add_property_set().
>Or actually, in linux-next there is already helper for platform bus called
>platform_device_add_properties().
>
>But I don't think there is any need for this kind of detail. You can just identify your
>platform in 8250_dw.c based on the ACPI ID, no?

Agree.

The uart and uart dma could work well, if I just set dma.rx_size in dw8250_quirks. 

Thank you for your comments.  

Regards,
Hongcheng(Annie)