Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753111AbbLJKdp (ORCPT ); Thu, 10 Dec 2015 05:33:45 -0500 Received: from mga03.intel.com ([134.134.136.65]:20201 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751272AbbLJKdn (ORCPT ); Thu, 10 Dec 2015 05:33:43 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,407,1444719600"; d="diff'?scan'208";a="615761163" Date: Thu, 10 Dec 2015 12:33:38 +0200 From: Heikki Krogerus To: Noam Camus Cc: "linux-kernel@vger.kernel.org" , "linux-serial@vger.kernel.org" , "gregkh@linuxfoundation.org" , "jslaby@suse.com" , "peter@hurleysoftware.com" , "fransklaver@gmail.com" , "Alexey.Brodkin@synopsys.com" , "vgupta@synopsys.com" Subject: Re: [PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses Message-ID: <20151210103338.GE4884@kuha.fi.intel.com> References: <1444113590-19722-1-git-send-email-noamc@ezchip.com> <1445158908-9610-1-git-send-email-noamc@ezchip.com> <20151209131939.GA14285@kuha.fi.intel.com> <20151210093104.GD4884@kuha.fi.intel.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="y0ulUmNC+osPPQO6" Content-Disposition: inline In-Reply-To: <20151210093104.GD4884@kuha.fi.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5068 Lines: 156 --y0ulUmNC+osPPQO6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Dec 10, 2015 at 11:31:04AM +0200, Heikki Krogerus wrote: > Hi Noam, > > On Thu, Dec 10, 2015 at 07:26:42AM +0000, Noam Camus wrote: > > >From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > > >Sent: Wednesday, December 09, 2015 3:20 PM > > > > > > >> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value) > > >> if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR)) > > >> return; > > >> dw8250_force_idle(p); > > >> - writel(value, p->membase + (UART_LCR << p->regshift)); > > >> + d->serial_out(value, > > >> + p->membase + (UART_LCR << p->regshift)); > > >> } > > > > >.. The way I see it, this is the only place where you would really need the > > >new private serial_in/out hooks, so why not just check the >iotype here and > > >based on that use writeb/writel/iowrite32be or what ever .. > > > > In previous versions (V2) Greg dis-liked using iotype here this is why I added > > the private serial_in/serial_out > > I couldn't find that comment in the thread? All he said in his > comment for this was that you should use real if condition instead of > the ternary operator you had there (condition ? true : false). > > Why would it not be acceptable? If it would really not be acceptable > (which I don't believe) then you should simply duplicate the lcr > checking to dw8250_seria_out32be like it is done now in > dw8250_serial_out and dw8250_serial_out32, but there still is no need > for the private serial_in/out hooks. > > I'm attaching a diff that I think would work as a good starting point > for you. Now actually attaching the diff :). -- heikki --y0ulUmNC+osPPQO6 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="dw8250_check_lcr.diff" diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index a5d319e..b2ea0c2 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -95,25 +95,39 @@ static void dw8250_force_idle(struct uart_port *p) (void)p->serial_in(p, UART_RX); } +static void dw8250_check_lcr(struct uart_port *p, int value) +{ + void __iomem *offset = p->membase + (UART_LCR << p->regshift); + int tries = 1000; + + while (tries--) { + unsigned int lcr = p->serial_in(p, UART_LCR); + + if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR)) + return; + + dw8250_force_idle(p); + + if (p->iotype == UPIO_MEM32) + writel(value, offset); + else + writeb(value, offset); + } + /* + * FIXME: this deadlocks if port->lock is already held + * dev_err(p->dev, "Couldn't set LCR to %d\n", value); + */ +} + static void dw8250_serial_out(struct uart_port *p, int offset, int value) { + struct dw8250_data *d = p->private_data; + writeb(value, p->membase + (offset << p->regshift)); /* Make sure LCR write wasn't ignored */ - if (offset == UART_LCR) { - int tries = 1000; - while (tries--) { - unsigned int lcr = p->serial_in(p, UART_LCR); - if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR)) - return; - dw8250_force_idle(p); - writeb(value, p->membase + (UART_LCR << p->regshift)); - } - /* - * FIXME: this deadlocks if port->lock is already held - * dev_err(p->dev, "Couldn't set LCR to %d\n", value); - */ - } + if (offset == UART_LCR && !d->uart_16550_compatible) + dw8250_check_lcr(p, value); } static unsigned int dw8250_serial_in(struct uart_port *p, int offset) @@ -161,23 +175,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value) static void dw8250_serial_out32(struct uart_port *p, int offset, int value) { + struct dw8250_data *d = p->private_data; + writel(value, p->membase + (offset << p->regshift)); /* Make sure LCR write wasn't ignored */ - if (offset == UART_LCR) { - int tries = 1000; - while (tries--) { - unsigned int lcr = p->serial_in(p, UART_LCR); - if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR)) - return; - dw8250_force_idle(p); - writel(value, p->membase + (UART_LCR << p->regshift)); - } - /* - * FIXME: this deadlocks if port->lock is already held - * dev_err(p->dev, "Couldn't set LCR to %d\n", value); - */ - } + if (offset == UART_LCR && !d->uart_16550_compatible) + dw8250_check_lcr(p, value); } static unsigned int dw8250_serial_in32(struct uart_port *p, int offset) @@ -463,10 +467,8 @@ static int dw8250_probe(struct platform_device *pdev) dw8250_quirks(p, data); /* If the Busy Functionality is not implemented, don't handle it */ - if (data->uart_16550_compatible) { - p->serial_out = NULL; + if (data->uart_16550_compatible) p->handle_irq = NULL; - } if (!data->skip_autocfg) dw8250_setup_port(p); --y0ulUmNC+osPPQO6-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/