marvell_nand driver fails to suspend

[miquel.raynal@xxxxxxxxxxx (Miquel Raynal)]

Hi Daniel,

Daniel Mack <daniel at zonque.org> wrote on Fri, 6 Jul 2018 10:41:35 +0200:

> Hi Miquel,
> 
> On Friday, July 06, 2018 10:27 AM, Miquel Raynal wrote:
> > Daniel Mack <daniel at zonque.org> wrote on Thu, 5 Jul 2018 10:22:29  
> >> I can't easily, because I don't have the individual timing
> >> parameters at hand. I could of course re-engineer them from the
> >> NDTR0/NDTR1 values the bootloader sets, but that would only lead to
> >> these registers being restored to the same value as they currently
> >> have. And as they're read during probe() anyway and re-used in
> >> marvell_nfc_select_chip(), I don't think that's the problem.
> > > I do agree with the second part of your statement. However I don't > understand why you talk about re-engineering the timings.  
> 
> The timings were calculated when I wrote the bootloader some years back, and all I'm saying is that at least for now, the kernel doesn't need to do that again. It should instead just take whatever has been provided by the bootloader.
> 
> > This > "keep-config" DT property was just an ugly hack to avoid
> > implementing ->setup_data_interface() and rely on the Bootloader's
> > setup. While this work at the slowest speed, it is clearly not as
> > efficient as tuning the timings at probe time depending on the NAND
> > chip. I recently changed the DT NAND nodes to 1/ separate the
> > controller and the NAND chip and 2/ remove superfluous properties
> > like this one.  
> 
> Do you have patches ready for that?

They are in the official tree now, the one I pointed below does this
for armada-38x which is I think the SoC you use?

925d5e426861 ARM: dts: armada-38x: update NAND node with new bindings

> 
> > If you are against NAND throughput, I suggest you to
> > test without it :)  
> 
> I have no problem adding the timing properties to DT,

I do :)

> but I don't expect that to improve the throughput. The bootloader is specifically written for this board, and it has optimized timing values already.

Ok, that was just an FYI.

> 
> >> /* * Do not change the timing registers when using the DT property >> * marvell,nand-keep-config; in that case ->ndtr0 and ->ndtr1 from
> >> the * marvell_nand structure are supposedly empty. */ >> writel_relaxed(marvell_nand->ndtr0, nfc->regs + NDTR0); >> writel_relaxed(marvell_nand->ndtr1, nfc->regs + NDTR1);
> > > This comment should not be there anymore. The logic described in the > first sentence is true and already handled in  
> > marvell_nand_chip_init(). But at any moment ->ndtr[0|1] are clearly
> > not supposed to be empty (anymore).  
> 
> But it's okay to write them here, that's what you're saying?

It's not just "ok", it's needed!

BTW, when I say "->ndtr[0|1]" I mean "the ndtr fields in the driver
structure", not "the physical registers".

> For the resume case, this is actually exactly what's needed, as the NDTR registers contents are lost in low-power mode.

The way you handle the timings looks good. Having selected_chip to
NULL will force the next call of ->select_chip() (done by the core,
please check this happens with a printk) to rewrite the NDTR registers
with valid saved values.

> 
> [...]
> 
> >> The controller loses all its state after resume, but I don't see
> >> which register is not re-initialized after that, but I might miss
> >> something.
> > > I suppose you are good with the above hooks.
> > > Maybe you can add traces in the IRQ handler and also dump NDSR on > timeout. Let's check: - If the IRQ is fired or not - If the right bit  
> > is set or not  
> 
> Yes, I did that, and the ISR is never entered after resume.

Is the interrupt bit really enabled and the RB bit set or not on
timeout? Can you dump NDCR and NDSR for that purpose?

> 
> > If it worked with the pxa3xx_nand.c driver, then it's probably a
> > driver issue... Can you test with the pxa3xx_nand.c driver with the
> > same kernel version?  
> 
> That's be my next attempt, yes. Thanks for pointing me to the right commits :)
> 
> > I have some platforms with such controller but I never used > suspend/resume on them, I'll have to check.  
> 
> It would be interesting to see if this works on other platforms, yes.
> 
> 
> Thanks,
> Daniel
> 

Thanks,
Miqu?l