Am Dienstag, den 06.01.2015, 00:41 +0100 schrieb Stefan Agner:
> Hi Lucas,
>
> Thanks for picking that up!
>
> I did some short benchmarks on Colibri T20 V1.2, L4T. Write/read speeds
> I measured on the YAFFS2 based file system:
>
> # dd if=/dev/zero of=test bs=50M count=1 conv=fdatasync
> 1+0 records in
> 1+0 records out
> 52428800 bytes (52 MB) copied, 9.88293 s, 5.3 MB/s
>
> echo 3 > /proc/sys/vm/drop_caches
> # dd if=test of=/dev/zero bs=50M count=1
> 1+0 records in
> 1+0 records out
> 52428800 bytes (52 MB) copied, 5.97056 s, 8.8 MB/s
>
Thanks, this puts things into perspective.
> So your values look quite realistic then!
>
> Some comments below...
>
> On 2015-01-04 21:39, Lucas Stach wrote:
> > Add support for the NAND flash controller found on NVIDIA
> > Tegra 2/3 SoCs. This is a largely reworked version of the driver
> > started by Thierry.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Lucas Stach <dev@xxxxxxxxxx>
> > ---
> > I've tested this driver with the in-kernel mtd-tests and some
> > realworld workloads on a Colibri T20 module.
> > ---
> > .../bindings/mtd/nvidia,tegra20-nand.txt | 30 +
> > MAINTAINERS | 6 +
> > drivers/mtd/nand/Kconfig | 6 +
> > drivers/mtd/nand/Makefile | 1 +
> > drivers/mtd/nand/tegra_nand.c | 794 +++++++++++++++++++++
> > 5 files changed, 837 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
> > create mode 100644 drivers/mtd/nand/tegra_nand.c
> >
[...]
> > +static irqreturn_t tegra_nand_irq(int irq, void *data)
> > +{
> > + struct tegra_nand *nand = data;
> > + irqreturn_t ret = IRQ_HANDLED;
> > + u32 isr, dma;
> > +
> > + isr = readl(nand->regs + ISR);
> > + dma = readl(nand->regs + DMA_CTRL);
> > +
> > + if (!isr && !(dma & DMA_CTRL_IS_DONE)) {
> > + ret = IRQ_NONE;
> > + goto out;
>
> The out label doesn't do anything more than just return. Why not just
> return IRQ_NONE here and return IRQ_HANDLED at the end, saves the local
> variable and helps readability...
>
Yes, this function looked a bit more complicated when I started to clean
this. So not removing the out label was just an oversight.
> Why is this needed anyway, is the IRQ shared with other peripherals?
>
> In the L4T driver, there is a warning message about spurious interrupts,
> does this works around this interrupts?
>
I haven't seen any spurious IRQs during my testing. So not doing
anything special should be okay I think.
[...]
> > +}
> > +
> > +static void tegra_nand_setup_timing(struct tegra_nand *nand, int mode)
> > +{
> > + unsigned long rate = clk_get_rate(nand->clk) / 1000000;
> > + unsigned long period = 1000000 / rate;
>
> Hm, period of a clock in ns... Sounds like a common use case. I searched
> for a macro/helper, but did not found anything. Well then.
>
> > + const struct nand_sdr_timings *timings;
> > + u32 val, reg = 0;
> > +
> > + timings = onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > + val = max3(timings->tAR_min, timings->tRR_min,
> > + timings->tRC_min) / period;
> > + if (val > 2)
> > + val -= 2;
>
> According to my TRM this is:
> Generated timing = (n+3) * NAND_CLK_PERIOD ns.
>
> Shouldn't this look like
> if (val >= 3)
> val -= 3;
> then?
>
> I see that all the calculations of the timings below are different then
> in TRM. I think we need to take the whole offset into account, or do I
> miss something here?
>
>
> > + reg |= TIMING_TCR_TAR_TRR(val);
> > +
> > + val = max(max(timings->tCS_min, timings->tCH_min),
> > + max(timings->tALS_min, timings->tALH_min)) / period;
> > + if (val > 1)
> > + val -= 1;
> > + reg |= TIMING_TCS(val);
>
> See mask error in macro definition.
>
> > +
> > + val = max(timings->tRP_min, timings->tREA_max) + 6000;
> > + reg |= TIMING_TRP(val / 1000);
> > + reg |= TIMING_TRP_RESP(val / period);
> > +
> > + reg |= TIMING_TWB(timings->tWB_max / period);
> > + reg |= TIMING_TWHR(timings->tWHR_min / period);
> > + reg |= TIMING_TWH(timings->tWH_min / 1000);
> > + reg |= TIMING_TWP(timings->tWP_min / 1000);
> > + reg |= TIMING_TRH(timings->tRHW_min / 1000);
>
> Why 1000 for those three values? In my TRM, those values are in
> NAND_CLK_PERIOD too.
>
Thanks for noticing. I wrote that part half a year ago and don't know
the details anymore. I will look this up again (and fix if necessary)
and come back to you.
Regards,
Lucas
--
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
