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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
