Re: [PATCH v3 4/6] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Dmitry Osipenko]

On Monday, 11 June 2018 14:50:13 MSK Boris Brezillon wrote:
> On Mon, 11 Jun 2018 14:45:45 +0300
> 
> Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
> > On Sunday, 10 June 2018 18:32:02 MSK Boris Brezillon wrote:
> > > On Sun, 10 Jun 2018 18:00:06 +0300
> > > 
> > > Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
> > > > > >> That seems a lot of work for a code path I do not intend to ever
> > > > > >> use
> > > > > >> 
> > > > > >> :-)
> > > > > > 
> > > > > > Are you sure that resetting HW resets the timing and other
> > > > > > registers
> > > > > > configuration? Reset implementation is HW-specific, like for
> > > > > > example
> > > > > > in a
> > > > > > case of a video decoder the registers state is re-intialized on HW
> > > > > > reset,
> > > > > > but registers configuration is untouched in a case of resetting
> > > > > > GPU.
> > > > > > I'd
> > > > > > suggest to check whether NAND controller resetting affects the HW
> > > > > > configuration.
> > > > > 
> > > > > It seems all registers are set back to their documented reset value:
> > > > > 
> > > > > [boot loader/ROM initialized values]
> > > > > [    1.270253] tegra-nand 70008000.nand: Tegra NAND controller
> > > > > register
> > > > > dump
> > > > > [    1.277051] tegra-nand 70008000.nand: COMMAND: 0x66880104
> > > > > [    1.282457] tegra-nand 70008000.nand: STATUS: 0x00000101
> > > > > [    1.287763] tegra-nand 70008000.nand: ISR: 0x01000120
> > > > > [    1.292818] tegra-nand 70008000.nand: IER: 0x00000000
> > > > > [    1.297863] tegra-nand 70008000.nand: CONFIG: 0x00840000
> > > > > [    1.303181] tegra-nand 70008000.nand: TIMING: 0x05040000
> > > > > [    1.308486] tegra-nand 70008000.nand: TIMING2: 0x00000003
> > > > > [    1.313897] tegra-nand 70008000.nand: CMD_REG1: 0x00000000
> > > > > [    1.319377] tegra-nand 70008000.nand: CMD_REG2: 0x00000030
> > > > > [    1.324868] tegra-nand 70008000.nand: ADDR_REG1: 0x03000000
> > > > > [    1.330435] tegra-nand 70008000.nand: ADDR_REG2: 0x00000000
> > > > > [    1.336011] tegra-nand 70008000.nand: DMA_MST_CTRL: 0x04100004
> > > > > [    1.341838] tegra-nand 70008000.nand: DMA_CFG_A: 0x00000fff
> > > > > [    1.347415] tegra-nand 70008000.nand: DMA_CFG_B: 0x0000001b
> > > > > [    1.352991] tegra-nand 70008000.nand: FIFO_CTRL: 0x0000aa00
> > > > > [reset]
> > > > > [    1.358559] tegra-nand 70008000.nand: Tegra NAND controller
> > > > > register
> > > > > dump
> > > > > [    1.365352] tegra-nand 70008000.nand: COMMAND: 0x00800004
> > > > > [    1.370744] tegra-nand 70008000.nand: STATUS: 0x00000101
> > > > > [    1.376060] tegra-nand 70008000.nand: ISR: 0x00000100
> > > > > [    1.381105] tegra-nand 70008000.nand: IER: 0x00000000
> > > > > [    1.386161] tegra-nand 70008000.nand: CONFIG: 0x10030000
> > > > > [    1.391466] tegra-nand 70008000.nand: TIMING: 0x00000000
> > > > > [    1.396782] tegra-nand 70008000.nand: TIMING2: 0x00000000
> > > > > [    1.402174] tegra-nand 70008000.nand: CMD_REG1: 0x00000000
> > > > > [    1.407664] tegra-nand 70008000.nand: CMD_REG2: 0x00000000
> > > > > [    1.413156] tegra-nand 70008000.nand: ADDR_REG1: 0x00000000
> > > > > [    1.418722] tegra-nand 70008000.nand: ADDR_REG2: 0x00000000
> > > > > [    1.424297] tegra-nand 70008000.nand: DMA_MST_CTRL: 0x24000000
> > > > > [    1.430123] tegra-nand 70008000.nand: DMA_CFG_A: 0x00000000
> > > > > [    1.435698] tegra-nand 70008000.nand: DMA_CFG_B: 0x00000000
> > > > > [    1.441264] tegra-nand 70008000.nand: FIFO_CTRL: 0x0000aa00
> > > > 
> > > > Alright, then indeed it's not really worth to bother with HW resetting
> > > > here. Probably only a kernel module reload or a reboot will help if HW
> > > > is
> > > > hung. Maybe NAND controller / chip recovering is something that NAND
> > > > core
> > > > should be handling in a such case by providing a
> > > > nand_controller_reset()
> > > > hook?
> > > 
> > > I don't see what the core could do to help with that. We'd end up with
> > > a new hook implemented by the controller that would be called by the
> > > controller driver when it knows it's safe to reset the controller. So,
> > > why bother exposing that in the core?
> > 
> > Giving a driver more flexibility is always a good thing. I'm not really
> > familiar with mtd/ and maybe indeed it doesn't make much sense to move HW
> > resetting to NAND core, though it looked to me that it should be always
> > safe for NAND core to initiate HW resetting after IO failure and hence
> > would be cleaner and nicer to have a unified HW reset management rather
> > than to have each driver to do its own thing.
> 
> No really, the NAND core can't know when it's appropriate to reset the
> controller, and what this reset will do, hence it doesn't know if the
> chips connected to the controller should also be reset.

Okay!