From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sekhar Nori Subject: Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA Date: Mon, 13 Jul 2015 14:39:12 +0530 Message-ID: <55A38038.8000801@ti.com> References: <9f70a47010d019f76b822b60e7d4f512aa4e46d7.1436174801.git.nsekhar@ti.com> <559DCF83.8010703@hurleysoftware.com> <559E8209.9080005@ti.com> <55A040A5.1080909@hurleysoftware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A040A5.1080909-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Peter Hurley , Greg Kroah-Hartman , Tony Lindgren Cc: Linux OMAP Mailing List , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Device Tree Mailing List , John Ogness , Sebastian Andrzej Siewior , linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Peter, On Saturday 11 July 2015 03:31 AM, Peter Hurley wrote: > On 07/09/2015 10:15 AM, Sekhar Nori wrote: >> On Thursday 09 July 2015 07:03 AM, Peter Hurley wrote: > > [...] > >>>> @@ -1307,6 +1320,36 @@ static int omap8250_runtime_suspend(struct device *dev) >>>> return -EBUSY; >>>> } >>>> >>>> + if (priv->habit & UART_ERRATA_CLOCK_DISABLE) { >>>> + int sysc; >>>> + int syss; >>>> + int timeout = 100; >>>> + >>>> + sysc = serial_in(up, UART_OMAP_SYSC); >>>> + >>>> + /* softreset the UART */ >>>> + sysc |= OMAP_UART_SYSC_SOFTRESET; >>>> + serial_out(up, UART_OMAP_SYSC, sysc); >>>> + >>>> + /* By experiments, 1us enough for reset complete on AM335x */ >>>> + do { >>>> + udelay(1); >>>> + syss = serial_in(up, UART_OMAP_SYSS); >>>> + } while (--timeout && !(syss & OMAP_UART_SYSS_RESETDONE)); >>> >>> >>> If there is not a omap helper function to reset modules, there should be. >>> Open-coding the module reset is not appropriate. >> >> There is work planned to have something implemented for OMAP as part of >> drivers/reset/ API. AFAIK, this depends on some PRCM code clean-up >> affecting multiple OMAP platforms and is couple of merge windows away >> from taking shape. >> >> Meanwhile, I think this is the best we can do. Other drivers like >> i2c-omap have done something similar (see omap_i2c_reset()). > > Ok, then please make the reset a separate local function > (returning success/failure?). omap_8250_reset()? > > A TODO or FIXME above it explaining > the temporary nature of the function would be appreciated :) Okay, will do. > >>> >>>> + if (!timeout) { >>>> + dev_err(dev, "timed out waiting for reset done\n"); >>>> + return -ETIMEDOUT; >>>> + } >>>> + >>>> + /* >>>> + * For UART module wake-up to work, MDR1.MODESELECT should >>>> + * not be set to "Disable", so update it. >>>> + */ >>>> + if (device_may_wakeup(dev)) >>>> + omap8250_update_mdr1(up, priv); >>> >>> Should this be unconditional? >> >> I do not see it doing any harm if done unconditionally. But it will be >> unnecessary. Unless the UART is being used for wakeup, we don't need >> MDR1 restored at this stage. And omap8250_restore_regs() will eventually >> restore the register anyway. > > Yeah, I understand that, but special-casing it isn't adding any value; > it's not as if you actually want the module to not be in UART mode. > > And the comment could be single-line: > > /* Restore to UART mode after reset (for wakeup) */ Alright, will change this as well. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html