On 09.06.2018 08:41, Boris Brezillon wrote:

On Sat, 09 Jun 2018 08:23:51 +0200
Stefan Agner [off-list ref] wrote:

quoted

On 09.06.2018 07:52, Boris Brezillon wrote:

quoted

On Fri, 08 Jun 2018 23:51:01 +0200
Stefan Agner [off-list ref] wrote:

quoted

quoted

void tegra_nand_controller_reset(struct tegra_nand_controller *ctrl)
{
	int err;

	disable_irq(ctrl->irq);

	err = reset_control_reset(ctrl->rst);
	if (err) {
		dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
		msleep(HW_TIMEOUT);
	}

	writel_relaxed(NAND_CMD_STATUS, ctrl->regs + HWSTATUS_CMD);
	writel_relaxed(HWSTATUS_MASK, ctrl->regs + HWSTATUS_MASK);
	writel_relaxed(INT_MASK, ctrl->regs + ISR);

If we do a controller reset, there is much more state than that which
needs to be restored. A lot of it is not readily available currently
(timing, ECC settings...)

This is actually a good test to detect what is not properly initialized
by the driver. Timings should be configured correctly through
->setup_data_interface(). ECC engine should be disabled by default and
only enabled when ->{read,write}_page() is called.

Is setup_data_interface guaranteed to be called after a failed
->exec_op()/{read,write}_page()?

No. Maybe I misunderstood when tegra_nand_controller_reset() was
supposed to be called. That's something I would call only once, early
in the probe function, so that the controller is placed in a well-known
state before we start using it. Definitely not something you should
call after each error.

Dmitry suggests to make use of it in the error handling path in case the
command/DMA  timed out.

Which makes sense in general I guess, just to make sure that the state
is properly set. It just isn't entirely trivial to do, since state is
setup during probe, chip detect and timing setup...

quoted

quoted

quoted

That seems a lot of work for a code path I do not intend to ever use :-)

Not so sure it's a lot of work. If ECC and timing settings are the
only thing you need to initialize then it should work just fine.
Try with a controller reset and you'll know if you miss something ;-).

Currently the setting gets written directly to the registers. Only the
enable flag is set in the HW ECC {read,write}_page() functions. So I
will have to store the complete register in the chip structure and write
them on every {read,write}_page()?

Well, your solution works as long as you only have one chip connected
to the controller. What we usually set the ECC config in
->select_chip() (or at least make sure the current setting matches the
one we expect) and then enable the engine in read/write_page() (as you
seem to already do).

I did not plan to make the driver multi chip capable, as I am not aware
of any real hardware using it.

But to properly reset state, we would have to have all the chip settings
stored somewhere...

--
Stefan