Re: [PATCH 2/2] mtd: nand: driver for Conexant Digicolor NAND Flash Controller

Hi Boris,

On Thu, Feb 19, 2015 at 11:52:23AM +0100, Boris Brezillon wrote:

On Thu, 19 Feb 2015 11:43:01 +0200
Baruch Siach [off-list ref] wrote:

quoted

On Thu, Feb 19, 2015 at 12:17:04AM +0100, Boris Brezillon wrote:

quoted

On Thu, 12 Feb 2015 13:10:19 +0200
Baruch Siach [off-list ref] wrote:

I understand you concern. However, since I don't have the needed hardware 
to test the multi nand chip setup I want to keep the code simple. These 
two fields are separated from the others by a blank line on purpose, and 
I'll also add a comment to explain that these are per nand chip files.

I'm not asking you to support multiple chips right now.
What I'm asking here is to dispatch NFC and NAND fields in different
structures:

struct digicolor_nand {
	struct mtd_info		mtd;
	struct nand_chip	nand;

	u32 nand_cs;
	int t_ccs;
}

struct digicolor_nfc {
	struct nand_hw_control	base;
	void __iomem		*regs;
	struct device		*dev;

	unsigned long		clk_rate;

	u32 ale_cmd;
	u32 ale_data;
	int ale_data_bytes;

	/* Only support one NAND for now */
	struct digicolor_nand	nand;
};

You'll keep the same logic except that this separation will be clearly
visible.

Sounds reasonable. Will do.

[...]

quoted

It's just that the INT flag name is quite long, and it make the code using 
it less readable IMO. Maybe some macro trickery would do the job here.

Yep, if that's about avoiding over 80 character lines you could define
such a macro:

#define NFC_RDY(flag)		NFC_INT_ ## flag ## _READY

Thanks for the tip. Will do.

[...]

quoted

I have no control of that. This command goes into a pipe that is managed 
at the hardware level. If the nand device does not activate the ready/busy 
signal, the pipe will just stall, and the command FIFO will overflow (the 
command FIFO has 8 entries). So you will notice the error eventually.

My point is: you should not return 1 if the NAND is not ready,
waiting forever is not what I'm suggesting here, but you should find a
way to return the actual NAND chip R/B status.
If you don't have any solution to do that from your controller then you
might consider relying on the default dev_ready implementation which is
sending a NAND STATUS command to retrieve the R/B status.

What I'll say is in contradiction with what's done in the sunxi
driver :-) (actually I'm considering fixing that), but after taking a
closer look, dev_ready should only return the status of the NAND, and
not wait for the NAND to be ready.
The function responsible for waiting is waitfunc, maybe this is the one
you should overload.

OK. I'll look into overloading waitfunc.

[...]

quoted

quoted

quoted

+static void digicolor_nfc_cmd_ctrl(struct mtd_info *mtd, int cmd,
+				   unsigned int ctrl)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct digicolor_nfc *nfc = chip->priv;
+	u32 cs = nfc->nand_cs;
+
+	if (ctrl & NAND_CLE) {
+		digicolor_nfc_cmd_write(nfc,
+					CMD_CLE | CMD_CHIP_ENABLE(cs) | cmd);
+		if (cmd == NAND_CMD_RNDOUTSTART || cmd == NAND_CMD_RNDIN) {
+			digicolor_nfc_wait_ns(nfc, nfc->t_ccs);
+		} else if (cmd == NAND_CMD_RESET) {
+			digicolor_nfc_wait_ns(nfc, 200);
+			digicolor_nfc_dev_ready(mtd);
+		}

These wait and dev_ready calls are supposed to be part of the generic
cmdfunc implementation, did you encounter any issues when relying on the
default implementation ?

The generic code just waits. This doesn't help here. Hardware does all the 
command processing in its own schedule. To make the hardware wait I must 
explicitly insert a wait command into the pipe.

I'm not sure to understand here.
There's a difference between:
1/ waiting for a NAND command to be send on the NAND bus
2/ waiting for the NAND to be ready (for DATA read or after a PROGRAM
   operation)

NAND core is taking care of the 2/, but 1/ is your responsibility
(and this is what you have to wait for here).

The generic code in nand_command_lp() sends NAND_CMD_STATUS immediately after 
NAND_CMD_RESET. According to the datasheet you must wait tWB (200ns max) 
before sending any command, including NAND_CMD_STATUS. Setting chip_delay is 
not enough as I explain above. That's way I added digicolor_nfc_wait_ns(). 
That timed wait is what I referred to. You are right that the 
digicolor_nfc_dev_ready() call is not needed here. Will remove.

[...]

quoted

quoted

quoted

+	while (len >= 4) {
+		if (digicolor_nfc_wait_ready(nfc, op))
+			return;
+		if (op == DATA_READ)
+			*pr++ = readl_relaxed(nfc->regs + NFC_DATA);
+		else
+			writel_relaxed(*pw++, nfc->regs + NFC_DATA);
+		len -= 4;
+	}

How about using readsl/writesl here (instead of this loop) ?

How can I add the wait condition in readsl/writesl?

Are you sure you have to wait after each readl access (is NFC_DATA
interfaced with a FIFO or directly doing PIO access) ?

That's what the datasheet says. There is no mention of data FIFO in the 
datasheet.

[...]

quoted

quoted

quoted

+	/*
+	 * Maximum assert/deassert time; asynchronous SDR mode 0
+	 * Deassert time = max(tWH,tREH) = 30ns
+	 * Assert time = max(tRC,tRP,tWC,tWP) = 100ns
+	 * Sample time = 0
+	 */
+	timing |= TIMING_DEASSERT(DIV_ROUND_UP(30, ns_per_clk));
+	timing |= TIMING_ASSERT(DIV_ROUND_UP(100, ns_per_clk));
+	timing |= TIMING_SAMPLE(0);
+	writel_relaxed(timing, nfc->regs + NFC_TIMING_CONFIG);
+	writel_relaxed(NFC_CONTROL_ENABLE, nfc->regs + NFC_CONTROL);

Helper functions are provided to extract timings from ONFI timing modes
(either those defined by the chip if it supports ONFI commands, or
those extracted from the datasheet):

http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L976

I wanted to keep that for future improvement. The NAND chip on the EVK board 
is actually an old style one, neither ONFI nor JEDEC. I expect to test this 
driver on our target hardware that should have something newer.

Timings are not only related to ONFI chips, and
onfi_timing_mode_default is extracted from the nand_ids table which is
describing non-ONFI chips.

This is not my call to make, but IMHO, dynamic NAND timing
configuration should be mandatory for new drivers.

I'll look into this for v2.

quoted

quoted

quoted

+static int digicolor_nfc_ecc_init(struct digicolor_nfc *nfc,
+				  struct device_node *np)
+{
+	struct mtd_info *mtd = &nfc->mtd;
+	struct nand_chip *chip = &nfc->nand;
+	int bch_data_range, bch_t, steps, mode, i;
+	u32 ctrl = NFC_CONTROL_ENABLE | NFC_CONTROL_BCH_KCONFIG;
+	struct nand_ecclayout *layout;
+
+	mode = of_get_nand_ecc_mode(np);
+	if (mode < 0)
+		return mode;
+	if (mode != NAND_ECC_HW_SYNDROME) {
+		dev_err(nfc->dev, "unsupported ECC mode\n");
+		return -EINVAL;
+	}
+
+	bch_data_range = of_get_nand_ecc_step_size(np);
+	if (bch_data_range < 0)
+		return bch_data_range;
+	if (bch_data_range != 512 && bch_data_range != 1024) {
+		dev_err(nfc->dev, "unsupported nand-ecc-step-size value\n");
+		return -EINVAL;
+	}
+	if (bch_data_range == 1024)
+		ctrl |= NFC_CONTROL_BCH_DATA_FIELD_RANGE_1024;
+	steps = mtd->writesize / bch_data_range;
+
+	bch_t = of_get_nand_ecc_strength(np);
+	if (bch_t < 0)
+		return bch_t;

You should fallback to datasheet values (ecc_strength_ds and
ecc_step_ds) if ECC strength and step are not specified in the DT.

I can only choose from a fix number of strength configuration alternatives. 
Should I choose the lowest value that is larger than ecc_strength_ds?

Yep, this applies to both cases (information extracted from the DT and
_ds values).

OK. Will do.

Thanks again for your valuable feedback,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html