| Message ID | 9f70a47010d019f76b822b60e7d4f512aa4e46d7.1436174801.git.nsekhar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sekhar, Patch title should start "serial: 8250_omap: .." since the patch is specific to the 8250_omap serial driver. 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 > 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. 2. Explain why SCR register has to be the context loss canary rather than MDR1. > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > --- > drivers/tty/serial/8250/8250_omap.c | 55 +++++++++++++++++++++++++++++++++---- > 1 file changed, 49 insertions(+), 6 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > index 52566387ec37..af25869d145f 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -33,6 +33,11 @@ > #define UART_ERRATA_i202_MDR1_ACCESS (1 << 0) > #define OMAP_UART_WER_HAS_TX_WAKEUP (1 << 1) > #define OMAP_DMA_TX_KICK (1 << 2) > +/* > + * See Advisory 21 in AM437x errata SPRZ408B, updated April 2015. > + * The same errata is applicable to AM335x and DRA7x processors too. > + */ > +#define UART_ERRATA_CLOCK_DISABLE (1 << 3) > > #define OMAP_UART_FCR_RX_TRIG 6 > #define OMAP_UART_FCR_TX_TRIG 4 > @@ -54,6 +59,12 @@ > #define OMAP_UART_MVR_MAJ_SHIFT 8 > #define OMAP_UART_MVR_MIN_MASK 0x3f > > +/* SYSC register bitmasks */ > +#define OMAP_UART_SYSC_SOFTRESET (1 << 1) > + > +/* SYSS register bitmasks */ > +#define OMAP_UART_SYSS_RESETDONE (1 << 0) > + > #define UART_TI752_TLR_TX 0 > #define UART_TI752_TLR_RX 4 > > @@ -1062,13 +1073,15 @@ static int omap8250_no_handle_irq(struct uart_port *port) > return 0; > } > > -static const u8 am3352_habit = OMAP_DMA_TX_KICK; > +static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE; > +static const u8 am4372_habit = UART_ERRATA_CLOCK_DISABLE; > > static const struct of_device_id omap8250_dt_ids[] = { > { .compatible = "ti,omap2-uart" }, > { .compatible = "ti,omap3-uart" }, > { .compatible = "ti,omap4-uart" }, > { .compatible = "ti,am3352-uart", .data = &am3352_habit, }, > + { .compatible = "ti,am4372-uart", .data = &am4372_habit, }, > {}, > }; > MODULE_DEVICE_TABLE(of, omap8250_dt_ids); > @@ -1279,13 +1292,13 @@ static int omap8250_lost_context(struct uart_8250_port *up) > { > u32 val; > > - val = serial_in(up, UART_OMAP_MDR1); > + val = serial_in(up, UART_OMAP_SCR); > /* > - * If we lose context, then MDR1 is set to its reset value which is > - * UART_OMAP_MDR1_DISABLE. After set_termios() we set it either to 13x > - * or 16x but never to disable again. > + * If we lose context, then SCR is set to its reset value of zero. > + * After set_termios() we set bit 3 of SCR (TX_EMPTY_CTL_IT) to 1, > + * among other bits, to never set the register back to zero again. > */ > - if (val == UART_OMAP_MDR1_DISABLE) > + if (!val) > return 1; > return 0; > } > @@ -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. > + 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? Regards, Peter Hurley > + } > + > if (up->dma && up->dma->rxchan) > omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT); > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 52566387ec37..af25869d145f 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -33,6 +33,11 @@ #define UART_ERRATA_i202_MDR1_ACCESS (1 << 0) #define OMAP_UART_WER_HAS_TX_WAKEUP (1 << 1) #define OMAP_DMA_TX_KICK (1 << 2) +/* + * See Advisory 21 in AM437x errata SPRZ408B, updated April 2015. + * The same errata is applicable to AM335x and DRA7x processors too. + */ +#define UART_ERRATA_CLOCK_DISABLE (1 << 3) #define OMAP_UART_FCR_RX_TRIG 6 #define OMAP_UART_FCR_TX_TRIG 4 @@ -54,6 +59,12 @@ #define OMAP_UART_MVR_MAJ_SHIFT 8 #define OMAP_UART_MVR_MIN_MASK 0x3f +/* SYSC register bitmasks */ +#define OMAP_UART_SYSC_SOFTRESET (1 << 1) + +/* SYSS register bitmasks */ +#define OMAP_UART_SYSS_RESETDONE (1 << 0) + #define UART_TI752_TLR_TX 0 #define UART_TI752_TLR_RX 4 @@ -1062,13 +1073,15 @@ static int omap8250_no_handle_irq(struct uart_port *port) return 0; } -static const u8 am3352_habit = OMAP_DMA_TX_KICK; +static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE; +static const u8 am4372_habit = UART_ERRATA_CLOCK_DISABLE; static const struct of_device_id omap8250_dt_ids[] = { { .compatible = "ti,omap2-uart" }, { .compatible = "ti,omap3-uart" }, { .compatible = "ti,omap4-uart" }, { .compatible = "ti,am3352-uart", .data = &am3352_habit, }, + { .compatible = "ti,am4372-uart", .data = &am4372_habit, }, {}, }; MODULE_DEVICE_TABLE(of, omap8250_dt_ids); @@ -1279,13 +1292,13 @@ static int omap8250_lost_context(struct uart_8250_port *up) { u32 val; - val = serial_in(up, UART_OMAP_MDR1); + val = serial_in(up, UART_OMAP_SCR); /* - * If we lose context, then MDR1 is set to its reset value which is - * UART_OMAP_MDR1_DISABLE. After set_termios() we set it either to 13x - * or 16x but never to disable again. + * If we lose context, then SCR is set to its reset value of zero. + * After set_termios() we set bit 3 of SCR (TX_EMPTY_CTL_IT) to 1, + * among other bits, to never set the register back to zero again. */ - if (val == UART_OMAP_MDR1_DISABLE) + if (!val) return 1; return 0; } @@ -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 (!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); + } + if (up->dma && up->dma->rxchan) omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT);
AM335x, AM437x and DRA7x SoCs have an errata due to which UART cannot be disabled after it has been used with DMA. 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. 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. Signed-off-by: Sekhar Nori <nsekhar@ti.com> --- drivers/tty/serial/8250/8250_omap.c | 55 +++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 6 deletions(-)