From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Osipenko Subject: Re: [PATCH v3 4/6] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver Date: Mon, 11 Jun 2018 16:10:21 +0300 Message-ID: <6764263.x6FQro3A0J@dimapc> References: <20180531221637.6017-1-stefan@agner.ch> <1862022.905gDpZtpO@dimapc> <20180611135013.7bb5b59b@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: In-Reply-To: <20180611135013.7bb5b59b@bbrezillon> Sender: linux-kernel-owner@vger.kernel.org To: Boris Brezillon Cc: Stefan Agner , dwmw2@infradead.org, computersforpeace@gmail.com, marek.vasut@gmail.com, robh+dt@kernel.org, mark.rutland@arm.com, thierry.reding@gmail.com, dev@lynxeye.de, miquel.raynal@bootlin.com, richard@nod.at, marcel@ziswiler.com, krzk@kernel.org, benjamin.lindqvist@endian.se, jonathanh@nvidia.com, pdeschrijver@nvidia.com, pgaikwad@nvidia.com, mirza.krak@gmail.com, linux-mtd@lists.infradead.org, linux-tegra@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-tegra@vger.kernel.org On Monday, 11 June 2018 14:50:13 MSK Boris Brezillon wrote: > On Mon, 11 Jun 2018 14:45:45 +0300 > > Dmitry Osipenko wrote: > > On Sunday, 10 June 2018 18:32:02 MSK Boris Brezillon wrote: > > > On Sun, 10 Jun 2018 18:00:06 +0300 > > > > > > Dmitry Osipenko 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!