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

[Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>]

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 :)

>>
>>> +		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) */

Regards,
Peter Hurley

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