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) -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
