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