From: Naga Sureshkumar Relli <nagasure@xilinx.com>
To: Helmut Grohne <helmut.grohne@intenta.de>
Cc: "bbrezillon@kernel.org" <bbrezillon@kernel.org>,
"richard@nod.at" <richard@nod.at>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"marek.vasut@gmail.com" <marek.vasut@gmail.com>,
"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
"miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
"nagasureshkumarrelli@gmail.com" <nagasureshkumarrelli@gmail.com>,
Michal Simek <michals@xilinx.com>,
"computersforpeace@gmail.com" <computersforpeace@gmail.com>,
"dwmw2@infradead.org" <dwmw2@infradead.org>
Subject: RE: [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Date: Wed, 27 Mar 2019 09:13:59 +0000 [thread overview]
Message-ID: <MWHPR02MB26235ED95B0F0AE89B2A0972AF580@MWHPR02MB2623.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20190326132721.52e7ktnptgwvzeor@laureti-dev>
Hi Helmut,
> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Sent: Tuesday, March 26, 2019 6:57 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: bbrezillon@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2019-03-27 9:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-09 6:37 [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353 smc nand interface Naga Sureshkumar Relli
2019-03-04 9:43 ` Miquel Raynal
2019-03-04 11:46 ` Naga Sureshkumar Relli
2019-03-11 4:36 ` Naga Sureshkumar Relli
2019-03-26 13:27 ` Helmut Grohne
2019-03-27 9:13 ` Naga Sureshkumar Relli [this message]
2019-03-28 11:51 ` Helmut Grohne
2019-03-29 5:21 ` Naga Sureshkumar Relli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR02MB26235ED95B0F0AE89B2A0972AF580@MWHPR02MB2623.namprd02.prod.outlook.com \
--to=nagasure@xilinx.com \
--cc=bbrezillon@kernel.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=helmut.grohne@intenta.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=michals@xilinx.com \
--cc=miquel.raynal@bootlin.com \
--cc=nagasureshkumarrelli@gmail.com \
--cc=richard@nod.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).