Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA

From: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
To: Peter Hurley
	<peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>,
	Greg Kroah-Hartman
	<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>,
	Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Cc: Linux OMAP Mailing List
	<linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>,
	Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
	Ian Campbell
	<ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>,
	Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
	Device Tree Mailing List
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	John Ogness <john.ogness-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>,
	Sebastian Andrzej Siewior
	<bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>,
	linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA
Date: Thu, 9 Jul 2015 19:45:37 +0530	[thread overview]
Message-ID: <559E8209.9080005@ti.com> (raw)
In-Reply-To: <559DCF83.8010703-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>

Hi Peter,

On Thursday 09 July 2015 07:03 AM, Peter Hurley wrote:
> Hi Sekhar,
> 
> Patch title should start "serial: 8250_omap: .." since the patch
> is specific to the 8250_omap serial driver.

Yes, missed that.

> 
> On 07/06/2015 05:47 AM, Sekhar Nori wrote:
>> AM335x, AM437x and DRA7x SoCs have an errata due to which UART
>> cannot be disabled after it has been used with DMA.
>              ^^^^^^
>              idled

alright.

> 
>> OMAP3 has a similar sounding errata which has been worked around
>> in a2fc36613ac1af2e9 ("ARM: OMAP3: Use manual idle for UARTs
>> because of DMA errata"). But the workaround used there does not
>> apply to AM335x, AM437x SoCs.
> 
>> These SoCs need a softreset of UART before disabling them.
> 
> "The UART module on these SoCs must be soft reset to go idle."
>  
>> This patch implements that errata workaround. It is expected that
>> UART will be used with DMA so no explicit check for DMA usage
>> has been added for errata applicability.
> 
> This changelog should also:
> 
> 1. Reference the errata document.

Sure. I added that as comments in code. I can do that in the patch
description too.

> 2. Explain why SCR register has to be the context loss canary
>    rather than MDR1.

Okay, will explain that too.

>> @@ -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()).

> 
>> +		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.

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