Hi jean-philippe,
On Thu, Jul 01, 2010 at 21:53:37, jean-philippe francois wrote:
> 2010/6/30 Sekhar Nori <[email protected]>:
> > This patch adds support to configure the AEMIF interface
> > with supplied timing values.
> >
>
> > Signed-off-by: Sekhar Nori <[email protected]>
> > ---
> > arch/arm/mach-davinci/Makefile | 2 +-
> > arch/arm/mach-davinci/aemif.c | 131
> > ++++++++++++++++++++++++++++
> > arch/arm/mach-davinci/include/mach/aemif.h | 36 ++++++++
> > 3 files changed, 168 insertions(+), 1 deletions(-)
> > create mode 100644 arch/arm/mach-davinci/aemif.c
> > create mode 100644 arch/arm/mach-davinci/include/mach/aemif.h
> >
> >
> > diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
> > new file mode 100644
> > index 0000000..62d7cce
> > --- /dev/null
> > +++ b/arch/arm/mach-davinci/aemif.c
> > @@ -0,0 +1,131 @@
> > +/*
> > + * AEMIF support for DaVinci SoCs
> > + *
> > + * Copyright (C) 2010 Texas Instruments Incorporated. http://www.ti.com/
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> > +
> > +#include <mach/aemif.h>
> > +
> > +/* Timing value configuration */
> > +
> > +#define TA(x) ((x) << 2)
> > +#define RHOLD(x) ((x) << 4)
> > +#define RSTROBE(x) ((x) << 7)
> > +#define RSETUP(x) ((x) << 13)
> > +#define WHOLD(x) ((x) << 17)
> > +#define WSTROBE(x) ((x) << 20)
> > +#define WSETUP(x) ((x) << 26)
> > +
> > +#define TA_MAX 0x3
> > +#define RHOLD_MAX 0x7
> > +#define RSTROBE_MAX 0x3f
> > +#define RSETUP_MAX 0xf
> > +#define WHOLD_MAX 0x7
> > +#define WSTROBE_MAX 0x3f
> > +#define WSETUP_MAX 0xf
> > +
> > +#define TIMING_MASK (TA(TA_MAX) | \
> > + RHOLD(RHOLD_MAX) | \
> > + RSTROBE(RSTROBE_MAX) | \
> > + RSETUP(RSETUP_MAX) | \
> > + WHOLD(WHOLD_MAX) | \
> > + WSTROBE(WSTROBE_MAX) | \
> > + WSETUP(WSETUP_MAX))
> > +
> > +#define NS_IN_KHZ 1000000
> > +
>
> Why are the defines splitted like this between the .h and the .c ? If
> users of this code
> don't care about these defines, they can be with the code.
> But I can't see how the defines in the header file are more useful to
> external code then these ones.
Defines in the header file are register offsets and register
bit defines which can be used by AEMIF users like NAND driver.
Defines in this file are expected to be used by this file only.
That said, this part of the patch can use some clean-up - I just
realized that A1CR_OFFSET is defined in nand.h as well so that
needs to be removed.
>
> > +/*
> > + * aemif_calc_rate - calculate timing data.
> > + * @wanted: The cycle time needed in nanoseconds.
> > + * @clk: The input clock rate in kHz.
> > + * @max: The maximum divider value that can be programmed.
> > + *
> > + * Returns the calculated timing value minus 1 for easy programming into
> > + * AEMIF timing registers.
> > + */
> > +static int aemif_calc_rate(int wanted, unsigned long clk, int max)
> > +{
> > + int result;
> > +
> > + result = DIV_ROUND_UP((wanted * clk), NS_IN_KHZ) - 1;
> > +
> > + pr_debug("%s: result %d from %ld, %d\n", __func__, result, clk,
> > wanted);
> > +
> > + if (result < 0)
> > + result = 0;
> > + else if (result > max)
> > + result = max;
> > +
> > + return result;
> > +}
>
> I think you should either return a negative value when result > max, or remove
> the check for negative value in the code below. I think a negative
> value would make more sense.
Right, as you can probably guess, there was a bit of a tussle on
which path to take and ended up with a bit of both.
[...]
> > diff --git a/arch/arm/mach-davinci/include/mach/aemif.h
> > b/arch/arm/mach-davinci/include/mach/aemif.h
> > new file mode 100644
> > index 0000000..05b2934
> > --- /dev/null
> > +++ b/arch/arm/mach-davinci/include/mach/aemif.h
> > @@ -0,0 +1,36 @@
> > +/*
> > + * TI DaVinci AEMIF support
> > + *
> > + * Copyright 2010 (C) Texas Instruments, Inc. http://www.ti.com/
> > + *
> > + * This file is licensed under the terms of the GNU General Public License
> > + * version 2. This program is licensed "as is" without any warranty of any
> > + * kind, whether express or implied.
> > + */
> > +#ifndef _MACH_DAVINCI_AEMIF_H
> > +#define _MACH_DAVINCI_AEMIF_H
> > +
> > +#define NRCSR_OFFSET 0x00
> > +#define AWCCR_OFFSET 0x04
> > +#define A1CR_OFFSET 0x10
> > +
> > +#define ACR_ASIZE_MASK 0x3
> > +#define ACR_EW_MASK BIT(30)
> > +#define ACR_SS_MASK BIT(31)
> > +
> > +/* All timings in nanoseconds */
> > +struct davinci_aemif_timing {
> > + u8 wsetup;
> > + u8 wstrobe;
> > + u8 whold;
> > +
> > + u8 rsetup;
> > + u8 rstrobe;
> > + u8 rhold;
> > +
> > + u8 ta;
> > +};
> > +
>
> So, maximum strobe time is 255 ns ?
> Looks a bit low for a max value.
As can be seen from patches 3-4, the max value
we are having to program is about 50ns. That
still leaves a lot of room. This can be changed
later if there is a need for more bits.
> Isn't this a good place to define chip select register base value ?
You mean the A1CR_OFFSET above? The AEMIF base address
comes to the drivers from platform data.
Thanks,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
