[U-Boot] [PATCH 1/6] am33xx: Enable UART{1,2,4,5} clocks

[Marek Vasut <marex@denx.de>]

Dear Tom Rini,

> On Fri, Oct 19, 2012 at 08:25:46PM -0400, Andrew Bradford wrote:
> > Tom & Marek,
> > 
> > On Thu, 27 Sep 2012 10:53:05 -0700
> > 
> > Tom Rini <trini@ti.com> wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > > 
> > > On 09/27/12 10:27, Marek Vasut wrote:
> > > > Dear Tom Rini,
> > > > 
> > > >> On 09/27/12 10:11, Marek Vasut wrote:
> > > >>> Dear Tom Rini,
> > > >>> 
> > > >>>> On 09/27/12 09:45, Marek Vasut wrote:
> > > >>>>> Dear Tom Rini,
> > > >>>>> 
> > > >>>>>> On Thu, Sep 27, 2012 at 06:13:36PM +0200, Marek Vasut
> > > >>>>>> 
> > > >>>>>> wrote:
> > > >>>>>>> Dear Andrew Bradford,
> > > >>>>>>> 
> > > >>>>>>>> If configured to use UART{1,2,4,5}, such as on the
> > > >>>>>>>> Beaglebone RS232 cape, enable the required clocks
> > > >>>>>>>> for the UART in use.
> > > >>>>>>>> 
> > > >>>>>>>> Signed-off-by: Andrew Bradford
> > > >>>>>>>> <andrew@bradfordembedded.com> ---
> > > >>>>>>>> 
> > > >>>>>>>> arch/arm/cpu/armv7/am33xx/clock.c |   28
> > > >>>>>>>> ++++++++++++++++++++++++++++ 1 file changed, 28
> > > >>>>>>>> insertions(+)
> > > >>>>>>>> 
> > > >>>>>>>> diff --git a/arch/arm/cpu/armv7/am33xx/clock.c
> > > >>>>>>>> b/arch/arm/cpu/armv7/am33xx/clock.c index
> > > >>>>>>>> 2b19506..4eb9226 100644 ---
> > > >>>>>>>> a/arch/arm/cpu/armv7/am33xx/clock.c +++
> > > >>>>>>>> b/arch/arm/cpu/armv7/am33xx/clock.c @@ -114,6
> > > >>>>>>>> +114,34 @@ static void enable_per_clocks(void)
> > > >>>>>>>> 
> > > >>>>>>>> while (readl(&cmwkup->wkup_uart0ctrl) !=
> > > >>>>>>>> PRCM_MOD_EN) ;
> > > >>>>>>>> 
> > > >>>>>>>> +	/* UART1 */ +#ifdef CONFIG_SERIAL2 +
> > > >>>>>>>> writel(PRCM_MOD_EN, &cmper->uart1clkctrl); +	while
> > > >>>>>>>> (readl(&cmper->uart1clkctrl) != PRCM_MOD_EN)
> > > >>>>>>>> +		;
> > > >>>>>>> 
> > > >>>>>>> Call WATCHDOG_RESET() here, fix glboally
> > > >>>>>> 
> > > >>>>>> We don't have WATCHDOG_RESET...
> > > >>>>> 
> > > >>>>> You do, and it opts-out to udelay(1) is most cases.
> > > >>>> 
> > > >>>> It looks like it opts-out to {} in most cases, in
> > > >>>> <watchdog.h>
> > > >>> 
> > > >>> Correct, we use it to retrigger watchdog timer if implemented.
> > > >> 
> > > >> Which the SoC support isn't doing and the rest of the code also
> > > >> isn't trying to use.  Arguably the whole file should be doing
> > > >> udelay(1) in each of these instances and a clean up patch which
> > > >> this series depends on might be useful.
> > > > 
> > > > So we're changing the practice from doing WATCHDOG_RESET() to
> > > > udelay(1) ? And we're doing so in generic code?
> > > 
> > > I think we should use WATCHDOG_RESET where it makes sense and udelay
> > > where we're just delaying.  I don't see WATCHDOG_RESET() being used
> > > for enable this-or-that clock.  But maybe I'm just really missing
> > > something about how we use WATCHDOG_RESET in the case where it's not a
> > > nop.
> > 
> > Is there a consensus on the use of WATCHDOG_RESET vs. udelay(1) in this
> > instance?
> > 
> > Looking through the arch/arm/cpu/armv7/{omap-common,omap3}/clock files
> > shows no use of either WATCHDOG_RESET or udelay(1) in the ways
> > mentioned. In fact, I can't easily find a use of WATCHDOG_RESET at all
> > within arch/arm/cpu/armv7 code.
> > 
> > What is the goal of using either udelay(1) or WATCHDOG_RESET as
> > recommended here?
> 
> Lets just match the rest of the SoC code and have the empty loop.  We
> can have the discussion about what kind of delay or macro makes most
> sense another time.

WATCHDOG_RESET() shall obviously be called, just enable watchdog and your uboot 
will keep restarting in such loops.

Best regards,
Marek Vasut