RE: [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Naga Sureshkumar Relli]

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@xxxxxxxxxx>
> Sent: Tuesday, March 26, 2019 6:57 PM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: bbrezillon@xxxxxxxxxx; miquel.raynal@xxxxxxxxxxx; richard@xxxxxx;
> dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> nagasureshkumarrelli@xxxxxxxxx
> Subject: Re: [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353 smc nand
> interface
> 
> On Sat, Feb 09, 2019 at 12:07:27PM +0530, Naga Sureshkumar Relli wrote:
> > +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> > +					bool force_8bit)
> > +{
> > +	struct pl353_nand_controller *xnfc =
> > +		container_of(chip, struct pl353_nand_controller, chip);
> 
> This `xnfc' variable is never used anywhere inside this function.
> 
> > +/**
> > + * pl353_nand_exec_op_cmd - Send command to NAND device
> > + * @chip:	Pointer to the NAND chip info structure
> > + * @subop:	Pointer to array of instructions
> > + * Return:	Always return zero
> > + */
> > +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> > +				  const struct nand_subop *subop) {
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	const struct nand_op_instr *instr;
> > +	struct pl353_nfc_op nfc_op = {};
> > +	struct pl353_nand_controller *xnfc =
> > +		container_of(chip, struct pl353_nand_controller, chip);
> > +	unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > +	unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> > +	unsigned int op_id, len, offset;
> > +	bool reading;
> > +
> > +	pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> > +	instr = nfc_op.data_instr;
> > +	op_id = nfc_op.data_instr_idx;
> > +
> > +	offset = nand_subop_get_data_start_off(subop, op_id);
> 
> This `offset' variable is never used anywhere inside this function. The call is unnecessary and
> should be removed.
Will remove it.
> 
> Beyond being useless, it also is harmful. When applying this patch on top of a v5.1-rc2, this can
> be found in dmesg:
> 
> | ------------[ cut here ]------------
> | WARNING: CPU: 0 PID: 1 at
> | .../linux/drivers/mtd/nand/raw/nand_base.c:2299
> | nand_subop_get_data_start_off+0x30/0x6c
> | CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc2-dirty #3 Hardware
> | name: Xilinx Zynq Platform [<c001743c>] (unwind_backtrace) from
> | [<c00145a4>] (show_stack+0x18/0x1c) [<c00145a4>] (show_stack) from
> | [<c03afe98>] (dump_stack+0xa0/0xcc) [<c03afe98>] (dump_stack) from
> | [<c0021c10>] (__warn+0x10c/0x128) [<c0021c10>] (__warn) from
> | [<c0021d14>] (warn_slowpath_null+0x48/0x50) [<c0021d14>]
> | (warn_slowpath_null) from [<c02703f0>]
> | (nand_subop_get_data_start_off+0x30/0x6c)
> | [<c02703f0>] (nand_subop_get_data_start_off) from [<c0279fe0>]
> | (pl353_nand_exec_op_cmd+0x94/0x2f0)
> | [<c0279fe0>] (pl353_nand_exec_op_cmd) from [<c027025c>]
> | (nand_op_parser_exec_op+0x460/0x4cc)
> | [<c027025c>] (nand_op_parser_exec_op) from [<c026ee4c>]
> | (nand_reset_op+0x134/0x1a0) [<c026ee4c>] (nand_reset_op) from
> | [<c0270adc>] (nand_reset+0x60/0xbc) [<c0270adc>] (nand_reset) from
> | [<c0272410>] (nand_scan_with_ids+0x288/0x1600) [<c0272410>]
> | (nand_scan_with_ids) from [<c027974c>] (pl353_nand_probe+0xf8/0x1a0)
> | [<c027974c>] (pl353_nand_probe) from [<c025185c>]
> | (platform_drv_probe+0x3c/0x74) [<c025185c>] (platform_drv_probe) from
> | [<c024fd28>] (really_probe+0x278/0x400) [<c024fd28>] (really_probe)
> | from [<c024e440>] (bus_for_each_drv+0x68/0x9c) [<c024e440>]
> | (bus_for_each_drv) from [<c0250090>] (__device_attach+0xa8/0x11c)
> | [<c0250090>] (__device_attach) from [<c024e63c>]
> | (bus_probe_device+0x90/0x98) [<c024e63c>] (bus_probe_device) from
> | [<c024cf7c>] (device_add+0x3b4/0x5f0) [<c024cf7c>] (device_add) from
> | [<c02b91b8>] (of_platform_device_create_pdata+0x98/0xc8)
> | [<c02b91b8>] (of_platform_device_create_pdata) from [<c02beba8>]
> | (pl353_smc_probe+0x194/0x234) [<c02beba8>] (pl353_smc_probe) from
> | [<c0223a64>] (amba_probe+0x60/0x74) [<c0223a64>] (amba_probe) from
> | [<c024fd28>] (really_probe+0x278/0x400) [<c024fd28>] (really_probe)
> | from [<c025062c>] (device_driver_attach+0x60/0x68) [<c025062c>]
> | (device_driver_attach) from [<c02506bc>] (__driver_attach+0x88/0x180)
> | [<c02506bc>] (__driver_attach) from [<c024df98>]
> | (bus_for_each_dev+0x60/0x9c) [<c024df98>] (bus_for_each_dev) from
> | [<c024e894>] (bus_add_driver+0x10c/0x208) [<c024e894>]
> | (bus_add_driver) from [<c0250dcc>] (driver_register+0x80/0x114)
> | [<c0250dcc>] (driver_register) from [<c04e2194>]
> | (do_one_initcall+0x164/0x374) [<c04e2194>] (do_one_initcall) from
> | [<c04e2738>] (kernel_init_freeable+0x394/0x474)
> | [<c04e2738>] (kernel_init_freeable) from [<c03c9660>]
> | (kernel_init+0x14/0x100) [<c03c9660>] (kernel_init) from [<c00090ac>]
> | (ret_from_fork+0x14/0x28) Exception stack(0xdd8c9fb0 to 0xdd8c9ff8)
> | 9fa0:                                     00000000 00000000 00000000 00000000
> | 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> | 00000000
> | 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 irq event
> | stamp: 414355 hardirqs last  enabled at (414361): [<c007c318>]
> | console_unlock+0x4c4/0x690 hardirqs last disabled at (414366):
> | [<c007bf30>] console_unlock+0xdc/0x690 softirqs last  enabled at
> | (414350): [<c000a4cc>] __do_softirq+0x454/0x544 softirqs last disabled
> | at (414345): [<c0027d98>] irq_exit+0x124/0x128 ---[ end trace
> | 3be9247df2f8dfb5 ]---
> 
> After removing the call (and the variable), this particular problem goes away.
Ok, will update
> 
> > +/**
> > + * pl353_nand_probe - Probe method for the NAND driver
> > + * @pdev:	Pointer to the platform_device structure
> > + *
> > + * This function initializes the driver data structures and the hardware.
> > + * The NAND driver has dependency with the pl353_smc memory
> > +controller
> > + * driver for initializing the NAND timing parameters, bus width, ECC
> > +modes,
> > + * control and status information.
> > + *
> > + * Return:	0 on success or error value on failure
> > + */
> > +static int pl353_nand_probe(struct platform_device *pdev) {
> > +	struct pl353_nand_controller *xnfc;
> > +	struct mtd_info *mtd;
> > +	struct nand_chip *chip;
> > +	struct resource *res;
> > +	struct device_node *np, *dn;
> > +	u32 ret, val;
> 
> This `val' variable is never used anywhere inside this function.
> 
> Even after fixing these, I couldn't make this driver work on actual hardware.
It's a on-die ECC capable device. Did u mentioned nand-ecc-mode = "on-die" in dts.
The same part I tested by mentioning "on-die" property in dts and it worked for me.
Please share the dts entries for NAND.
Also if it is x8 bus then please mention nand-bus-width = <8>;
If it is x16 mention nand-bus-width = <16>;

> 
> | nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
> | nand: Micron MT29F2G08ABAEAWP
> | nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
> | nand: WARNING: MT29F2G08ABAEAWP: the ECC used on your system is too
> | weak compared to the one required by the NAND chip
> | pl353_nand_calculate_hwecc status failed pl353_nand_calculate_hwecc
> | status failed pl353_nand_calculate_hwecc status failed
> | pl353_nand_calculate_hwecc status failed Bad block table not found for
> | chip 0 pl353_nand_calculate_hwecc status failed
> | pl353_nand_calculate_hwecc status failed pl353_nand_calculate_hwecc
> | status failed pl353_nand_calculate_hwecc status failed Bad block table
> | not found for chip 0
> 
> The very same device works fine with an older version of the out-of-tree driver based on a
> v4.14 tree. Thus far I couldn't figure out why it fails like this.
> 
> I'd appreciate if you could Cc me on future postings of this driver.
Sure, I will put you in CC.

On this v13 patch, got comments from Miquel to remove legacy hooks.
I will update the driver and will send next version.

Thanks,
Naga Sureshkumar Relli
> 
> Helmut